Problem/Motivation

Currently entity type schema is created only when installing the module providing the definitions. However it may happen that a new definition is added in an already installed module. Currently the related schema will never be created nor it will be detected as a missing schema update.

Proposed resolution

Make the EntityDefinitionUpdateManager detect also new entity type definitions and apply the related updates.

Remaining tasks

  • Validate the proposed solution
  • Write a patch
  • Review it

User interface changes

None

API changes

None

CommentFileSizeAuthor
#10 entity-on_create-2423213-10-test.patch4.89 KBplach
#10 entity-on_create-2423213-10.patch15.97 KBplach
#1 entity-on_create-2423213-1.patch9.57 KBplach
#2 entity-on_create-2423213-2.review.txt5.38 KBplach
#7 entity-on_create-2423213-7.interdiff.txt1.51 KBplach
#7 entity-on_create-2423213-7.patch11.08 KBplach
#13 entity-on_create-2423213-13.interdiff.txt790 bytesplach
#13 entity-on_create-2423213-13.patch15.82 KBplach
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Status: Active » Needs review
FileSize
entity-on_create-2423213-1.patch9.57 KB

Let's see how many things break.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
FileSize
entity-on_create-2423213-2.review.txt5.38 KB

A diff -w version to ease reviewing.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Related issues: +#2282119: Make the Entity Field API handle field purging

Status: Needs review » Needs work

The last submitted patch, 1: entity-on_create-2423213-1.patch, failed testing.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Issue tags: +D8 upgrade path
plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented

@catch:

I'm not sure this affects the upgrade path: once this is fixed, we just need to apply the regular db updates, I think.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Status: Needs work » Needs review
FileSize
entity-on_create-2423213-7.patch11.08 KB
entity-on_create-2423213-7.interdiff.txt1.51 KB

This should fix test failures, additional test coverage still to do.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

@plach doesn't this mean that:

1. Install a module
2. Module adds an entity type in a new release
3. Update that module
4. Site is hosed without manual intervention

If so I think the upgrade path tag is right, but feel free to untag if this isn't the case.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented

I meant the 4 can be fixed by just running update.php, if this is fixed when we already support the upgrade path.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Issue tags: -Needs tests
FileSize
entity-on_create-2423213-10-test.patch4.89 KB
entity-on_create-2423213-10.patch15.97 KB

And now with tests. Interdiff is the test-only patch.

The last submitted patch, 10: entity-on_create-2423213-10-test.patch, failed testing.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented 
  1. +++ b/core/modules/locale/src/Tests/LocaleTranslatedSchemaDefinitionTest.php
@@ -31,6 +31,8 @@ protected function setUp() {
     $this->config('system.site')->set('langcode', 'fr')->save();
+    // Make sure new entity type definitions are processed.
+    \Drupal::service('entity.definition_update_manager')->applyUpdates();
     // Clear all caches so that the base field definition, its cache in the
     // entity manager, the t() cache, etc. are all cleared.

    Why is this needed here? 

  2. +++ b/core/modules/system/tests/modules/entity_test/src/Entity/EntityTestNew.php
@@ -0,0 +1,35 @@
+ *   label = @Translation("Test entity new"),
+ *   handlers = {
+ *     "storage_schema" = "Drupal\entity_test\EntityTestStorageSchema"
+ *   },
+ *   base_table = "entity_test_new",
+ *   fieldable = TRUE,
+ *   persistent_cache = FALSE,

    I think we don't need the custom handler and fieldable/cache definitions, to keep this as short as possible?

    Also, I think "New test entity" reads a bit better?

RTBC apart from those nitpicks I think.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
FileSize
entity-on_create-2423213-13.patch15.82 KB
entity-on_create-2423213-13.interdiff.txt790 bytes

Why is this needed here?

Because also config entities trigger the on create event, and for some reason those are not registered in the entity manager state when setting up the test.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs review » Reviewed & tested by the community

Great, this looks good to me then.

alexpott’s picture

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

This issue addresses a critical bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 8007ab1 and pushed to 8.0.x. Thanks!

  • alexpott committed 8007ab1 on 8.0.x 
    Issue #2423213 by plach: Schema for newly defined entity types is never...
plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Assigned: plach » Unassigned
Issue tags: -sprint

Thanks!

Status: Fixed » Closed (fixed)

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

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Issue tags: -Needs Drupal 8 critical triage