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. :-)

CommentFileSizeAuthor
#12 entity-type-2302235-12.patch10.52 KBpushpinderchauhan
#12 interdiff-2302235-10-12.txt1.31 KBpushpinderchauhan
#1 entity-type-2302235-1.patch6.16 KBtim.plunkett
#8 entity-type-2302235-8.patch6.2 KBl0ke
#10 interdiff-2302235-8-10.txt5.91 KBl0ke
#10 entity-type-2302235-10.patch10.54 KBl0ke

Comments

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Active » Needs review
StatusFileSize
new entity-type-2302235-1.patch6.16 KB

Sure. Let's do them all I guess.

Crell’s picture

Crell commented
Status: Needs review » Reviewed & tested by the community

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

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Issue tags: +Quick fix

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

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work 
  1. +++ 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 

  2. +++ 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 :) 

  3. +++ 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 

  4. +++ 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

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Maybe NULL instead of FALSE?

Crell’s picture

Crell commented

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.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented

They're snake_case because they come from annotations.

l0ke’s picture

l0ke
Primary language English
Location Ukraine
commented
Status: Needs work » Needs review
StatusFileSize
new entity-type-2302235-8.patch6.2 KB

Changed according to review.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs review » Needs work 
  1. +++ 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. 

  2. +++ 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 

  3. +++ 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...

l0ke’s picture

l0ke
Primary language English
Location Ukraine
commented
Status: Needs work » Needs review
StatusFileSize
new entity-type-2302235-10.patch10.54 KB
new interdiff-2302235-8-10.txt5.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.

penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾 Seville 💃 Andalusia, UTC+2 🇪🇺
commented
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.

pushpinderchauhan’s picture

pushpinderchauhan commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-2302235-10-12.txt1.31 KB
new entity-type-2302235-12.patch10.52 KB

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

penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾 Seville 💃 Andalusia, UTC+2 🇪🇺
commented
Status: Needs review » Reviewed & tested by the community

Great, thanks!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

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

  • alexpott committed 2d03e83 on 8.0.x 
    Issue #2302235 by lokeoke, er.pushpinderrana, tim.plunkett | Crell: Set...
m1r1k’s picture

m1r1k commented
Issue tags: +#ams2014contest

Status: Fixed » Closed (fixed)

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

andypost’s picture

andypost
he/him
Primary language Russian
commented
Issue tags: -

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