Problem/Motivation

As discovered in #2984782-34: Add an API for converting entity type schemas to (non-)revisionable/(non-)translatable, with or without data, the current API for installing an entity type (EntityDefinitionUpdateManagerInterface::installEntityType($entity_type)) is insufficient because it doesn't allow developers to specify the field storage definitions that should be installed.

Quoting myself from that comment:

let's say in 8.7.0 we install a new content entity type in a hook_update_8701() function. The entity storage will use the "live" field storage definitions (from code) to install that entity type. Then in 8.8.0, we add a new base field to that entity type with a corresponding hook_update_8801() function. Now, if someone skips a minor version and updates from 8.6.x to 8.8.0 directly, which is not uncommon at all, hook_update_8701() will also install the new field added in 8.8.0 and hook_update_8801() will fail, unless the person who writes that update function is aware of all these shortcomings and makes the code as defensive as possible.

Proposed resolution

Add a new \Drupal\Core\Entity\EntityDefinitionUpdateManagerInterface::installFieldableEntityType($entity_type, $field_storage_definitions) method, similar to the update one that was added in #2984782: Add an API for converting entity type schemas to (non-)revisionable/(non-)translatable, with or without data.

Remaining tasks

Review.

User interface changes

Nope.

API changes

API addition: a new installFieldableEntityType($entity_type, $field_storage_definitions) method added to EntityDefinitionUpdateManagerInterface.

Data model changes

Nope.

Release notes snippet

Nope.

CommentFileSizeAuthor
#14 interdiff-14.txt5.64 KBamateescu
#14 3043455-14.patch19.36 KBamateescu
#14 interdiff-test-only.txt3.12 KBamateescu
#14 3043455-14-test-only.patch19.36 KBamateescu
#9 interdiff-9.txt3.47 KBamateescu
#9 3043455-9.patch16.77 KBamateescu
#8 interdiff-8.txt950 bytesamateescu
#8 3043455-8.patch13.96 KBamateescu
#6 3043455-6.patch13.97 KBamateescu
#4 interdiff-4.txt680 bytesamateescu
#4 3043455-4.patch13.96 KBamateescu
#2 3043455.patch13.29 KBamateescu
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

amateescu created an issue. See original summary.

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented
Status: Active » Needs review
FileSize
3043455.patch13.29 KB

This should do it.

Status: Needs review » Needs work

The last submitted patch, 2: 3043455.patch, failed testing. View results

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented
Status: Needs work » Needs review
FileSize
3043455-4.patch13.96 KB
interdiff-4.txt680 bytes

Now with less fails :)

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran as a volunteer commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs change record

This looks ready to me. Let's create a change record. Issue title seems like it is a new feature or a task but IS makes it clear that it is a bug. Should we consider updating the title?

+++ b/core/lib/Drupal/Core/Entity/EntityTypeListener.php
@@ -79,6 +79,27 @@ public function onEntityTypeCreate(EntityTypeInterface $entity_type) {
+    // @todo Forward this to all interested handlers, not only storage, once
+    //   iterating handlers is possible: https://www.drupal.org/node/2332857.

This is a blast from past.

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented
FileSize
3043455-6.patch13.97 KB

Just a reroll.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 6: 3043455-6.patch, failed testing. View results

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented
Status: Needs work » Reviewed & tested by the community
FileSize
3043455-8.patch13.96 KB
interdiff-8.txt950 bytes

Fixed that deprecated call.

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented
Priority: Normal » Major
Status: Reviewed & tested by the community » Needs review
Issue tags: +WI critical
FileSize
3043455-9.patch16.77 KB
interdiff-9.txt3.47 KB

Let's add even more test coverage by fixing an upgrade path function from 8.8.x.

plach’s picture

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

No change record yet :)

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented
Issue tags: -Needs change record

Here we go: https://www.drupal.org/node/3086279

The patch from #3007669-15: Add publishing status to path aliases is now passing tests when combined with this one :)

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran as a volunteer commented
Status: Needs review » Reviewed & tested by the community

Thanks, for the change notice.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach as a volunteer commented
Status: Reviewed & tested by the community » Needs review 
  1. +++ b/core/lib/Drupal/Core/Entity/EntityTypeListener.php
@@ -80,6 +80,27 @@ public function onEntityTypeCreate(EntityTypeInterface $entity_type) {
+    $this->eventDispatcher->dispatch(EntityTypeEvents::CREATE, new EntityTypeEvent($entity_type));

    Shouldn't we dispatch the event after updating the installed definitions as we do in ::onFieldableEntityTypeUpdate()? If so, we're missing some test coverage. 

  2. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
@@ -442,6 +412,43 @@ public function onEntityTypeDelete(EntityTypeInterface $entity_type) {
+    // When installing an entity type, we have to use the live (in-code) entity
+    // type and field storage definitions.

    This comment needs to be updated. 

  3. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityDefinitionUpdateTest.php
@@ -148,6 +148,26 @@ public function testEntityTypeUpdateWithEntityStorageChange() {
+    // Rename the base table, update the fieldable entity type and check that
+    // the table has been renamed.

    Is this comment a copy/paste?

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented
Status: Needs review » Reviewed & tested by the community
FileSize
3043455-14-test-only.patch19.36 KB
interdiff-test-only.txt3.12 KB
3043455-14.patch19.36 KB
interdiff-14.txt5.64 KB

@plach, re #13:

  1. Great catch! Fixed and provided a test-only interdiff and patch to show that the new test catches this problem.
  2. Done.
  3. Yup, it was copy/pasted from another test metohod, fixed.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 14: 3043455-14.patch, failed testing. View results

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented
Status: Needs work » Reviewed & tested by the community

The non-test-only patch had a random fail, setting back to RTBC.

plach’s picture

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

Yep, the random failure is tracked at #3086001: CreateSampleEntityTest::testSampleValueContentEntity can randomly fail, confirmed locally that test is green.

The interdiffs look great, thanks! I will wait for the testbot for good measure and then commit this :)

  • plach committed 24e2236 on 8.8.x 
    Issue #3043455 by amateescu, jibran: Add an API for installing a...
plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach as a volunteer commented
Status: Reviewed & tested by the community » Fixed

Committed 24e2236 and pushed to 8.8.x. Thanks!

Status: Fixed » Closed (fixed)

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