Problem/Motivation

The primary goal of the Workflow Initiative is to make most core entity types revisionable and publishable so they can be used in the context of Content Moderation, Workspaces, full-site previews, etc.

There is a long chain of issues that is trying to accomplish this, with the end result being that an entity type can be converted to revisionable (with revision metadata fields) and publishable in a minor Drupal core version (i.e. in 8.4.0)

Proposed resolution

Before we start converting entity types we need to be sure that we can actually do all these updates at once.

Remaining tasks

Have a green patch in this issue.

Also, this issue has the following dependencies:
#2721313: Upgrade path between revisionable / non-revisionable entities
#2856808: Break out the 'entity_test_update' entity type into its own module and add additional test db dumps
#2615496: A serial/primary key field can not be added to an existing table for some databases
#2346019: Handle initial values when creating a new field storage definition
#2854732: Use initial values for content translation metadata fields and fix existing data

User interface changes

Nope.

API changes

Field storage CRUD operations will now use the last installed entity type and field storage definitions.

Data model changes

Nope.

CommentFileSizeAuthor
#30 interdiff.txt4 KBamateescu
#30 2860654-30.patch21.76 KBamateescu
#27 interdiff.txt1.21 KBamateescu
#27 2860654-27.patch21.5 KBamateescu
#25 2860654-25.patch21.52 KBamateescu
#22 2860654-22-combined.patch32.67 KBamateescu
#22 2860654-22.patch23.4 KBamateescu
#21 interdiff.txt11.83 KBamateescu
#21 2860654-21-combined.patch51.93 KBamateescu
#21 2860654-21.patch23.4 KBamateescu
#19 interdiff.txt886 bytesamateescu
#19 2860654-18-combined.patch43.39 KBamateescu
#19 2860654-18.patch14.83 KBamateescu
#15 interdiff-15.txt10.32 KBamateescu
#15 2860654-15-combined.patch43.39 KBamateescu
#15 2860654-15.patch14.83 KBamateescu
#13 interdiff.txt4.15 KBamateescu
#13 2860654-13-combined.patch43.79 KBamateescu
#13 2860654-13.patch17.06 KBamateescu
#11 interdiff.txt5.97 KBamateescu
#11 2860654-11-combined.patch44.41 KBamateescu
#11 2860654-11.patch16.21 KBamateescu
#9 interdiff.txt8.06 KBamateescu
#9 2860654-9.patch17.54 KBamateescu
#3 2860654-combined.patch476.74 KBamateescu
#2 2860654.patch11.95 KBamateescu

Comments

amateescu created an issue. See original summary.

amateescu’s picture

amateescu commented
Status: Active » Needs review
StatusFileSize
new 2860654.patch11.95 KB

Here's an initial patch.

amateescu’s picture

amateescu commented
StatusFileSize
new 2860654-combined.patch476.74 KB

And one combined with all the dependencies.

The last submitted patch, 2: 2860654.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 3: 2860654-combined.patch, failed testing.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
Title: [PP-6] Add tests for converting an entity type to revisionable and publishable » [PP-5] Add tests for converting an entity type to revisionable and publishable

#2615496: A serial/primary key field can not be added to an existing table for some databases is in.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
Title: [PP-5] Add tests for converting an entity type to revisionable and publishable » [PP-4] Add tests for converting an entity type to revisionable and publishable

#2856808: Break out the 'entity_test_update' entity type into its own module and add additional test db dumps is also fixed.

amateescu’s picture

amateescu commented
Title: [PP-4] Add tests for converting an entity type to revisionable and publishable » [PP-3] Add tests for converting an entity type to revisionable and publishable

#2721313: Upgrade path between revisionable / non-revisionable entities was committed!!

amateescu’s picture

amateescu commented
Title: [PP-3] Add tests for converting an entity type to revisionable and publishable » [PP-2] Add tests for converting an entity type to revisionable and publishable
Status: Needs work » Needs review
StatusFileSize
new 2860654-9.patch17.54 KB
new interdiff.txt8.06 KB

We now depend only on #2346019: Handle initial values when creating a new field storage definition and #2854732: Use initial values for content translation metadata fields and fix existing data, so I got back to this issue and we finally have a good (and properly tested) way for making an entity type publishable and revisionable!

Status: Needs review » Needs work

The last submitted patch, 9: 2860654-9.patch, failed testing.

amateescu’s picture

amateescu commented
Status: Needs work » Needs review
StatusFileSize
new 2860654-11.patch16.21 KB
new 2860654-11-combined.patch44.41 KB
new interdiff.txt5.97 KB

The interdiff in #9 tried to fix all field storage operations (create, update, delete) but I quickly ended up in a can of worms because the entity update system is pretty much broken in regards to the correct usage of last installed definitions vs. actual runtime definitions of field storages and entity types.

So I scaled back the fix to only include the parts that we need for the scope of this issue, which is making sure that we can convert and entity type to be revisionable and add a published base field at the same time.

Note that this also includes @tstoeckler's fix for #2346019-49: Handle initial values when creating a new field storage definition, and I tried to keep the ugliness contained into a single place (\Drupal\Core\Entity\Sql\SqlContentEntityStorageSchema::onFieldStorageDefinitionCreate).

Status: Needs review » Needs work

The last submitted patch, 11: 2860654-11-combined.patch, failed testing.

amateescu’s picture

amateescu commented
Status: Needs work » Needs review
StatusFileSize
new 2860654-13.patch17.06 KB
new 2860654-13-combined.patch43.79 KB
new interdiff.txt4.15 KB

Ok then, let's try this.

Status: Needs review » Needs work

The last submitted patch, 13: 2860654-13-combined.patch, failed testing.

amateescu’s picture

amateescu commented
Status: Needs work » Needs review
StatusFileSize
new 2860654-15.patch14.83 KB
new 2860654-15-combined.patch43.39 KB
new interdiff-15.txt10.32 KB

Fingers crossed :D

amateescu’s picture

amateescu commented
Issue summary: View changes

Removed a dependency which is no longer necessary from the IS.

The last submitted patch, 15: 2860654-15.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 15: 2860654-15-combined.patch, failed testing.

amateescu’s picture

amateescu commented
Status: Needs work » Needs review
StatusFileSize
new 2860654-18.patch14.83 KB
new 2860654-18-combined.patch43.39 KB
new interdiff.txt886 bytes

Meh..

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented 
+++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
@@ -402,7 +402,20 @@ public function onEntityTypeDelete(EntityTypeInterface $entity_type) {
+    // @todo This should be fixed by https://www.drupal.org/node/2274017.

If this issue is test only then why not postpone it on #2274017: Make SqlContentEntityStorage table mapping agnostic ?

amateescu’s picture

amateescu commented
StatusFileSize
new 2860654-21.patch23.4 KB
new 2860654-21-combined.patch51.93 KB
new interdiff.txt11.83 KB

Because this issue is not test only, it's more like "do everything needed before we can convert entity types to revisionable and publishable" :)

Meanwhile, I've been working on the next patch in line and I realized that there are a lot of other field updates that will break if we don't use the last installed entity type definition in field storage CUD operations, so we need to go back to the initial approach from #11.

amateescu’s picture

amateescu commented
Title: [PP-2] Add tests for converting an entity type to revisionable and publishable » [PP-1] Make it possible to convert an entity type to revisionable and publishable
StatusFileSize
new 2860654-22.patch23.4 KB
new 2860654-22-combined.patch32.67 KB

One of the blocking issues is in, just one more to go :)

amateescu’s picture

amateescu commented
Title: [PP-1] Make it possible to convert an entity type to revisionable and publishable » [PP-1] FIeld storage CRUD operations must use the last install entity type and field definitions

Changing the title to show what this issue is actually about.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Status: Needs review » Needs work

Hehe, looks like you found #2884894: Remove obsolete + duplicate EntityTestUpdate class before me in #21. I don't mind doing this here but in any case we should also remove entity_test_entity_field_storage_info() together with the obsolete entity type. So up to you whether we close that one as a dupe or extract that part to there...

amateescu’s picture

amateescu commented
Title: [PP-1] FIeld storage CRUD operations must use the last install entity type and field definitions » FIeld storage CRUD operations must use the last install entity type and field definitions
Issue summary: View changes
Status: Needs work » Needs review
StatusFileSize
new 2860654-25.patch21.52 KB

@tstoeckler, very good point about entity_test_entity_field_storage_info(), and thanks for the separate issue, I moved over the deletion of the duplicate EntityTestUpdate class there so we can keep this patch focused on the problem described in the issue title.

#2854732: Use initial values for content translation metadata fields and fix existing data is in so this is now the last blocker for being able to convert entity types to revisionable and publishable! :D

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
commented
Status: Needs review » Reviewed & tested by the community

Looks like we're ready to go!

amateescu’s picture

amateescu commented
Title: FIeld storage CRUD operations must use the last install entity type and field definitions » Field storage CRUD operations must use the last installed entity type and field storage definitions
Category: Task » Bug report
Issue tags: +entity storage
StatusFileSize
new 2860654-27.patch21.5 KB
new interdiff.txt1.21 KB

Just a couple of cosmetic fixes to match what we did in the previous (blocker) issues.

Also, I'd love to have @plach and/or @effulgentsia in here for a quick sanity check of this patch.. :)

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Epic, epic work, @amateescu, @timmillwood and other Workflow Initiative contributors!

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented
Related issues: +#2554235: Make the content entity storage and entity query use the last installed definitions instead of the ones living in code

Looks great to me, a few minor remarks:

  1. +++ b/core/lib/Drupal/Core/Field/FieldStorageDefinitionListener.php
@@ -70,6 +71,16 @@ public function onFieldStorageDefinitionCreate(FieldStorageDefinitionInterface $
     $storage = $this->entityTypeManager->getStorage($entity_type_id);

@@ -89,6 +100,16 @@ public function onFieldStorageDefinitionUpdate(FieldStorageDefinitionInterface $
     $storage = $this->entityTypeManager->getStorage($entity_type_id);

@@ -108,6 +129,16 @@ public function onFieldStorageDefinitionDelete(FieldStorageDefinitionInterface $
     $storage = $this->entityTypeManager->getStorage($entity_type_id);

    Shouldn't we clone $storage to ensure the switch doesn't leak into other areas and causes unintended behaviors? 

  2. +++ b/core/modules/system/src/Tests/Update/EntityUpdateToRevisionableAndPublishableTest.php
@@ -0,0 +1,216 @@
+  /**
+   * The entity manager service.
+   *
+   * @var \Drupal\Core\Entity\EntityManagerInterface
+   */
+  protected $entityManager;

    Isn't this deprecated?

amateescu’s picture

amateescu commented
StatusFileSize
new 2860654-30.patch21.76 KB
new interdiff.txt4 KB

Thanks @plach, fixed both points from #29 :)

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented

RTBC +1, thanks

catch’s picture

catch
he/him
Primary language English
commented

Committed/pushed to 8.4.x, thanks!

  • catch committed db4cb40 on 8.4.x 
    Issue #2860654 by amateescu: Field storage CRUD operations must use the...
plach’s picture

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

:)

Status: Fixed » Closed (fixed)

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