Set default property values in EntityType [#2302235]

Currently, Drupal\Core\Entity\EntityType has these three methods:

  public function isStaticallyCacheable() {
    return isset($this->static_cache) ? $this->static_cache: TRUE;
  }

  public function isRenderCacheable() {
    return isset($this->render_cache) ? $this->render_cache: TRUE;
  }

  public function isFieldDataCacheable() {
    return isset($this->field_cache) ? $this->field_cache: TRUE;
  }

The isset() is necessary because the properties in question are set to NULL by default. However, that's silly. It's way better to just set $static_cache, $render_cache, and $field_cache to TRUE by default in their definitions, then return the properties directly in these methods.

Let's do that. :-)

Comment	File	Size	Author
#12	entity-type-2302235-12.patch	10.52 KB	er.pushpinderrana
#12
#12	interdiff-2302235-10-12.txt	1.31 KB	er.pushpinderrana
#1	entity-type-2302235-1.patch	6.16 KB	tim.plunkett
#1
#8	entity-type-2302235-8.patch	6.2 KB	l0ke
#8
#10	interdiff-2302235-8-10.txt	5.91 KB	l0ke
#10	entity-type-2302235-10.patch	10.54 KB	l0ke
#10

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

tim.plunkett

he/him

English

Philadelphia

CreditAttribution: tim.plunkett commented 13 July 2014 at 05:10

Status:

Active

» Needs review

File	Size
entity-type-2302235-1.patch	6.16 KB

Sure. Let's do them all I guess.

Comment #2

Crell CreditAttribution: Crell commented 13 July 2014 at 06:35

Status:

Needs review

» Reviewed & tested by the community

Hey, you're not a novice! :-P

Comment #3

tim.plunkett

he/him

English

Philadelphia

CreditAttribution: tim.plunkett commented 13 July 2014 at 06:42

Issue tags:

+Quick fix

Whoops, sorry :\
I just jumped at the EntityType part.

Comment #4

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott commented 13 July 2014 at 10:29

Status:

Reviewed & tested by the community

» Needs work

+++ b/core/lib/Drupal/Core/Entity/EntityType.php
@@ -91,14 +91,14 @@ class EntityType implements EntityTypeInterface {
    * @var bool (optional)
    */
-  protected $fieldable;
+  protected $fieldable = FALSE;

(optional) is weird

+++ b/core/lib/Drupal/Core/Entity/EntityType.php
@@ -112,56 +112,56 @@ class EntityType implements EntityTypeInterface {
    * @var string
    */
-  protected $label_callback;
+  protected $label_callback = FALSE;

@var string and then a default of FALSE :)

+++ b/core/lib/Drupal/Core/Entity/EntityType.php
@@ -112,56 +112,56 @@ class EntityType implements EntityTypeInterface {
    * @var string
    */
-  protected $bundle_of;
+  protected $bundle_of = FALSE;
...
    * @var string
    */
-  protected $bundle_label;
+  protected $bundle_label = FALSE;
...
    * @var string
    */
-  protected $base_table;
+  protected $base_table = FALSE;
...
    * @var string
    */
-  protected $revision_data_table;
+  protected $revision_data_table = FALSE;
...
    * @var string
    */
-  protected $revision_table;
+  protected $revision_table = FALSE;
...
    * @var string
    */
-  protected $data_table;
+  protected $data_table = FALSE;

@var string and a value of FALSE

+++ b/core/lib/Drupal/Core/Entity/EntityType.php
@@ -175,14 +175,14 @@ class EntityType implements EntityTypeInterface {
    * @var callable
    */
-  protected $uri_callback;
+  protected $uri_callback = FALSE;

@var string and default of FALSE

I think we should be doing @var string|FALSE

Comment #5

Berdir

German

Switzerland

CreditAttribution: Berdir commented 13 July 2014 at 14:44

Maybe NULL instead of FALSE?

Comment #6

Crell CreditAttribution: Crell commented 13 July 2014 at 16:27

For uri_callback, callable|null. Strings are only one of many callables. (Arrays are also callables, in the right circumstances, plus closures.)

Also, why are all of these properties snake_cased? Properties should be lowerCamel.

Comment #7

tim.plunkett

he/him

English

Philadelphia

CreditAttribution: tim.plunkett commented 13 July 2014 at 16:48

They're snake_case because they come from annotations.

Comment #8

l0ke

English

Ukraine

CreditAttribution: l0ke commented 23 July 2014 at 15:47

Status:

Needs work

» Needs review

File	Size
entity-type-2302235-8.patch	6.2 KB

Changed according to review.

Comment #9

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott commented 23 July 2014 at 16:07

Status:

Needs review

» Needs work

+++ b/core/lib/Drupal/Core/Entity/EntityType.php
@@ -91,14 +91,14 @@ class EntityType implements EntityTypeInterface {
    * @var string|bool
    */
-  protected $permission_granularity;
+  protected $permission_granularity = 'entity_type';

Doesn't look like this is ever a bool - needs checking though.

+++ b/core/lib/Drupal/Core/Entity/EntityType.php
@@ -112,56 +112,56 @@ class EntityType implements EntityTypeInterface {
    * @var string
    */
-  protected $label_callback;
+  protected $label_callback = NULL;
...
    * @var string
    */
-  protected $bundle_of;
+  protected $bundle_of = NULL;
...
    * @var string
    */
-  protected $bundle_label;
+  protected $bundle_label = NULL;
...
    * @var string
    */
-  protected $base_table;
+  protected $base_table = NULL;
...
    * @var string
    */
-  protected $revision_data_table;
+  protected $revision_data_table = NULL;
...
    * @var string
    */
-  protected $revision_table;
+  protected $revision_table = NULL;
...
    * @var string
    */
-  protected $data_table;
+  protected $data_table = NULL;

@var string|null

+++ b/core/lib/Drupal/Core/Entity/EntityType.php
@@ -175,14 +175,14 @@ class EntityType implements EntityTypeInterface {
    * @var callable
    */
-  protected $uri_callback;
+  protected $uri_callback = NULL;

@var callable|null

And we also need to check the documentation on the interface for all the methods...

  /**
   * Returns the name of the entity's revision table.
   *
   * @todo Used by ContentEntityDatabaseStorage only.
   *
   * @return string|bool
   *   The name of the entity type's revision table.
   */
  public function getRevisionTable();

Is now wrong...

Comment #10

l0ke

English

Ukraine

CreditAttribution: l0ke commented 23 July 2014 at 20:09

Status:

Needs work

» Needs review

File	Size
entity-type-2302235-10.patch	10.54 KB

interdiff-2302235-8-10.txt	5.91 KB

Thanks for review @alexpott. To the fist, I didn't find any references where $permission_granularity is bool.
A new patch and interdiff, considering your notes.

Comment #11

penyaskito

he/him

Spanish

🧑🏽‍🌾Seville 💃, Andalusia 🇪🇸, UTC+2 🇪🇺

CreditAttribution: penyaskito commented 10 August 2014 at 23:27

Status:

Needs review

» Needs work

+++ b/core/lib/Drupal/Core/Entity/EntityTypeInterface.php
@@ -563,8 +562,9 @@ public function getConfigPrefix();
+   *   The name of the entity type's revision data table,
+   *   or NULL if none exists.

@@ -573,8 +573,9 @@ public function getRevisionDataTable();
+   *   The name of the entity type's revision table,
+   *   or NULL if none exists.

@@ -583,8 +584,9 @@ public function getRevisionTable();
+   *   The name of the entity type's data table,
+   *   or NULL if none exists.

We should adjust this comments in one line (looks like they should fit). If needed, a break should be just after the last word before the 80 chars limit.

Comment #12

er.pushpinderrana CreditAttribution: er.pushpinderrana commented 11 August 2014 at 03:45

Status:

Needs work

» Needs review

File	Size
interdiff-2302235-10-12.txt	1.31 KB
entity-type-2302235-12.patch	10.52 KB

Just fixed doc comments as @penyaskito mentioned above, in this patch. Please review.

Comment #13

penyaskito

he/him

Spanish

🧑🏽‍🌾Seville 💃, Andalusia 🇪🇸, UTC+2 🇪🇺

CreditAttribution: penyaskito commented 11 August 2014 at 19:56

Status:

Needs review

» Reviewed & tested by the community

Great, thanks!

Comment #14

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott commented 12 August 2014 at 18:10

Status:

Reviewed & tested by the community

» Fixed

Committed 2d03e83 and pushed to 8.0.x. Thanks!

Comment #15

12 August 2014 at 18:10

alexpott committed 2d03e83 on 8.0.x

Issue #2302235 by lokeoke, er.pushpinderrana, tim.plunkett | Crell: Set...

Comment #16

m1r1k CreditAttribution: m1r1k commented 15 August 2014 at 14:55

Issue tags:

+#ams2014contest

Comment #17

29 August 2014 at 15:00

Status:

Fixed

» Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

Comment #18

andypost

he/him

Russian

CreditAttribution: andypost commented 29 September 2014 at 12:59

Issue tags:

Filed follow-up #2346857: Set default property bundle_entity_type in EntityType to NULL

Set default property values in EntityType

Comments

Comment #1

Comment #2

Comment #3

Comment #4

Comment #5

Comment #6

Comment #7

Comment #8

Comment #9

Comment #10

Comment #11

Comment #12

Comment #13

Comment #14

Comment #15

Comment #16

Comment #17

Comment #18

Child issues

Thank you to these Drupal contributors

News items

Our community

Documentation

Drupal code base

Governance of community