Make the content entity storage and entity query use the last installed definitions instead of the ones living in code

Had problem https://www.drupal.org/node/2655162 which is related to this issue

Opened #2976035: Entity type CRUD operations must use the last installed entity type and field storage definitions which is related, but kept as separate for now, as that is definitely a specific bug while is more a general issue.

Here's a small step for this: #3030532: Entity query should use the last installed entity type and field storage definitions

That issue is needed by #3030483: [PP-1] Make SqlFieldableEntityTypeListenerTrait storage-agnostic.

What I realized is that if we switch to the last installed definition *everywhere* then that is likely going to cause a lot of problems, because a ton of settings right now don't require update functions as they are not storage-centric. As a simple example, that would be non-storage handlers, plural labels and so on. all those things would simply be gone if we stopped using the actual plugin definitions at runtime.

I think that's not an issue for that specific new issues, entity query is very much storage-centric.. but we'll have to be very careful with this and can't just change EntityTypeManager::getDefinitions() for example like the issue title says right now :)

The recent patches for converting taxonomy terms and custom menu links to be revisionable in 8.7.x uncovered that we need to be able to do CRUD operations on entities *before* their schema is updated, which makes this issue critical because a lot of sites won't be able to update to 8.7.0 without it.

Here's a patch which adds the ability to get the last installed definitions for both the entity type and field storage definitions from the entity type manager and the entity field manager, and uses them in the content entity storage and entity queries.

Crediting @joelpittet and @Sam152 for reporting this in #3039586: Cannot rename tmp_2362aemenu_link_content_revision to menu_link_content_revision and #3040129: Fatals in make_menu_link_content_revisionable when combined with make_taxonomy_term_revisionable.

The test-only patch provides test coverage for both of the issues mentioned above.

Rewrote the IS and here's a CR: https://www.drupal.org/node/3040966

I ran #19 on a 8.7-dev install and I got the following hunk failed:

patching file core/core.services.yml
Hunk #1 succeeded at 555 (offset 3 lines).
patching file core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
patching file core/lib/Drupal/Core/Entity/EntityDefinitionUpdateManager.php
patching file core/lib/Drupal/Core/Entity/EntityFieldManager.php
patching file core/lib/Drupal/Core/Entity/EntityFieldManagerInterface.php
patching file core/lib/Drupal/Core/Entity/EntityManager.php
patching file core/lib/Drupal/Core/Entity/EntityTypeManager.php
patching file core/lib/Drupal/Core/Entity/EntityTypeManagerInterface.php
patching file core/lib/Drupal/Core/Entity/Query/Sql/Tables.php
Hunk #5 FAILED at 285.
Hunk #6 FAILED at 372.
Hunk #7 succeeded at 406 (offset 3 lines).
Hunk #8 succeeded at 434 (offset 3 lines).
2 out of 8 hunks FAILED -- saving rejects to file core/lib/Drupal/Core/Entity/Query/Sql/Tables.php.rej
patching file core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
patching file core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
patching file core/lib/Drupal/Core/Entity/Sql/SqlFieldableEntityTypeListenerTrait.php
patching file core/lib/Drupal/Core/Field/FieldStorageDefinitionListener.php
patching file core/modules/content_moderation/tests/src/Kernel/ContentModerationStateTest.php
patching file core/modules/workspaces/src/EntityQuery/Tables.php
patching file core/tests/Drupal/KernelTests/Core/Entity/FieldableEntityDefinitionUpdateTest.php
patching file core/tests/Drupal/Tests/Core/Entity/EntityManagerTest.php
patching file core/tests/Drupal/Tests/Core/Entity/EntityTypeManagerTest.php
patching file core/tests/Drupal/Tests/Core/Entity/Sql/SqlContentEntityStorageSchemaTest.php
patching file core/tests/Drupal/Tests/Core/Entity/Sql/SqlContentEntityStorageTest.php

Here is a reroll of #19 for 8.7.
Hope this help

@DuneBL, the patch from #19 was applied by the testbot on the 8.7.x branch and passed all the tests as well. There could be a problem with your local installation if it can't be applied there..

I assume you are right...
I have downloaded 8.7-dev today using:
wget https://ftp.drupal.org/files/projects/drupal-8.7-dev.tar.gz
Do you know how to get the version of the Drupal files I have downloaded?
In core/lib/Drupal.php, I can read const VERSION = '8.7.0-dev';

1. +++ b/core/lib/Drupal/Core/Entity/Query/Sql/Tables.php
   @@ -44,11 +44,18 @@ class Tables implements TablesInterface {
      /**
   -   * The entity manager.
   +   * The entity type manager.
       *
   -   * @var \Drupal\Core\Entity\EntityManager
   +   * @var \Drupal\Core\Entity\EntityTypeManagerInterface
       */
   -  protected $entityManager;
   +  protected $entityTypeManager;
   

   should we do the deprecated property thing here.

   This part also overlaps with my EntityFieldManager deprecation issue, will postpone it on this one I guess.

2. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
   @@ -165,8 +157,15 @@ public static function createInstance(ContainerInterface $container, EntityTypeI
   +   * @deprecated in Drupal 8.7.0, will be removed before Drupal 9.0.0.
   +   *   Use \Drupal\Core\Entity\EntityFieldManagerInterface::getActiveFieldStorageDefinitions()
   +   *   instead.
   +   *
   +   * @see https://www.drupal.org/node/3040966
       */
   ...
   +    @trigger_error('SqlContentEntityStorage::getFieldStorageDefinitions() is deprecated in Drupal 8.7.0 and will be removed before Drupal 9.0.0. Use \Drupal\Core\Entity\EntityFieldManagerInterface::getActiveFieldStorageDefinitions() instead. See https://www.drupal.org/node/3040966.', E_USER_DEPRECATED);
        return $this->entityFieldManager->getBaseFieldDefinitions($this->entityTypeId);
      }
   

   I guess this duplicates/solves #2584729: SqlContentEntityStorage::getFieldStorageDefinitions() is poorly named and should not be public as well, then.

3. +++ b/core/tests/Drupal/Tests/Core/Entity/Sql/SqlContentEntityStorageTest.php
   @@ -70,13 +69,6 @@ class SqlContentEntityStorageTest extends UnitTestCase {
       */
      protected $entityFieldManager;
    
   -  /**
   -   * The mocked entity last installed schema repository used in this test.
   -   *
   -   * @var \Drupal\Core\Entity\EntityLastInstalledSchemaRepositoryInterface|\PHPUnit_Framework_MockObject_MockObject
   -   */
   -  protected $entityLastInstalledSchemaRepository;
   -
      /**
   

   couldn't you have done this before I had to update all those tests :p

Thanks for taking a look!

1. Good point, done.
2. That's right, I just closed it as a duplicate.
3. And make you miss all the fun? :P

@DuneBL, I'm not sure how to apply this patch on a packaged version of Drupal, but if you follow the instructions for downloading it with Git, you should be able to apply it like any other patch :)

I was seeing the following error after updating to 8.7.0-alpha1 and running drush updb:
[2019-03-19 12:46:26] menu.ERROR: Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[42S22]: Column not found: 1054 Unknown column 'base_table.revision_id' in 'field list': SELECT base_table.revision_id AS revision_id, base_table.id AS id FROM {menu_link_content} base_table INNER JOIN {menu_link_content_data} menu_link_content_data ON menu_link_content_data.id = base_table.id WHERE menu_link_content_data.rediscover = :db_condition_placeholder_0; Array ( [:db_condition_placeholder_0] => 1 ) in Drupal\menu_link_content\Plugin\Deriver\MenuLinkContentDeriver->getDerivativeDefinitions() (line 63 of /data/app/core/modules/menu_link_content/src/Plugin/Deriver/MenuLinkContentDeriver.php). {"referer":"","ip":"127.0.0.1","request_uri":"http://127.0.0.1/","uid":0,"user":""}

After updating to 8.7.0-alpha1, applying the patch from #27 and running drush updb there was no error. FWIW, I'm using the drush version 9.6.0

I also reviewed the patch and didn't find anything objectionable so setting it to RTBC. Just one question.

+++ b/core/lib/Drupal/Core/Entity/EntityManager.php
@@ -209,6 +209,34 @@ public function getFieldStorageDefinitions($entity_type_id) {
+   * @deprecated in Drupal 8.0.0, will be removed before Drupal 9.0.0.
+   *   Use \Drupal\Core\Entity\EntityFieldManagerInterface::getActiveFieldStorageDefinitions()
...
+   * @deprecated in Drupal 8.0.0, will be removed before Drupal 9.0.0.
+   *   Use \Drupal\Core\Entity\EntityFieldManagerInterface::setActiveFieldStorageDefinitions()

@@ -795,6 +823,34 @@ public function hasDefinition($plugin_id) {
+   * @deprecated in Drupal 8.0.0, will be removed before Drupal 9.0.0.
+   *   Use \Drupal\Core\Entity\EntityTypeManagerInterface::getActiveDefinition()
...
+   * @deprecated in Drupal 8.0.0, will be removed before Drupal 9.0.0.
+   *   Use \Drupal\Core\Entity\EntityTypeManagerInterface::setActiveDefinition()

Why do we have to add these to EM?

That's because EntityManagerInterface extends EntityTypeManagerInterface and EntityFieldManagerInterface which have the new methods, so we need to provide an implementation for them.

I had a different error during update from 8.6 to 8.7.0-alpha1 that was also fixed by the patch in #2

[notice] Update started: menu_link_content_post_update_make_menu_link_content_revisionable
 [error]  SQLSTATE[42S02]: Base table or view not found: 1146 Table 'onesite.taxonomy_term_revision' doesn't exist: SELECT revision.revision_id AS revision_id, revision.langcode AS langcode, revision.revision_user AS revision_user, revision.revision_created AS revision_created, revision.revision_log_message AS revision_log_message, revision.revision_default AS revision_default, base.tid AS tid, base.vid AS vid, base.uuid AS uuid, CASE base.revision_id WHEN revision.revision_id THEN 1 ELSE 0 END AS isDefaultRevision
FROM
{taxonomy_term_data} base
INNER JOIN {taxonomy_term_revision} revision ON revision.revision_id = base.revision_id
WHERE base.tid IN (:db_condition_placeholder_0); Array
(
    [:db_condition_placeholder_0] => 1
)

 [error]  Update failed: menu_link_content_post_update_make_menu_link_content_revisionable
 [error]  Update aborted by: menu_link_content_post_update_make_menu_link_content_revisionable
 [error]  Finished performing updates.

Thanks @amateescu!

@plach, your rant got me thinking: since we're now using the last installed definitions in the storage handler and entity query, we can simplify the "data copy" step of fieldable entity type updates a bit and use the current storage object instead of trying to re-construct one that uses the original definitions.

This led me to the fact that we only need to create a "special" storage object when generating the new schema and re-saving the existing data to temporary tables, and we can do that by keeping a static cache of field storage definitions in the storage itself, with a setFieldStorageDefinitions() method to accompany the existing setEntityType(). This way we are able to keep the "ugliness" contained only in the storage handler instead of spilling it over into the entity type and field managers.

The only downside of this approach is that we have to be more aggressive with clearing the entity type manager cache after field storage CRUD operations (because we need to refresh the statically cached storage handler), which means that some unrelated entity handlers might get out of sync in functional tests, as shown by the changes needed in NodeAccessLanguageTest.

1. That's a nice suggestion, let's do it :)

2. The problem with relying only on EntityLastInstalledSchemaRepositoryInterface is that entity query needs to do it as well, and it will be a performance problem because the last installed repository does not have any static cache for its definitions. I tried to do this for a few hours today but it results in some failures in update path tests and I think it might be a risky change at this point :/

How about removing the new "get active" methods from their interfaces and marking them as @internal so we can iterate on them in 8.8.x?

3. Since we're keeping the get active methods, fixed this point.

In other news, JSON:API was committed yesterday and its tests are storing a storage object in a test class variable which gets out of sync as explained in #33, so we need to fix that here as well..

Not having those methods on the interfaces means that we need to mock the classes instead of the interfaces in unit tests.

Discussed a bit with @plach and we found a way of keeping the field storage definitions in sync that doesn't require any changes in (unrelated) tests.

+++ b/core/lib/Drupal/Core/Entity/EntityFieldManager.php
@@ -55,6 +55,13 @@ class EntityFieldManager implements EntityFieldManagerInterface {
+   * Static cache of active field storage definitions per entity type.

Off topic but it would be good to find another name for non-static class property caches than 'static cache'. Can only think of 'local cache' which is not necessarily better.

I can't see anything else to complain about, good to see us flushing these remaining issues out now rather than later.

Updated the IS and the CR now that we're not adding the two methods on the main interfaces.

I am working on an update path test on #3041659-24: Remove the layout tab from translations because Layout Builder does not support translations yet
Not sure why but it fails and just revertying this issue fixes it.

Any insight over on the other issue would be appreciated.

One interesting side-effect that came out of this issue that I've encountered was: I previously had a custom entity type that was missing the 'revision_table' annotation key. I didn't realise annotation changes like this had to be installed. After updating, EFQ complained that my entity type didn't have a revision table: #2928506: Entity query fails with allRevisions flag set when no explicit revision table name is provided..

There may be some influx of 8.7 issues where folks may not have correctly updated their entity types, where previously the in-code version made everything work roughly as expected.

Yes, there's a chance that this will result in some isuses, we've alredy seen some with previous changes where sites had unclean installed entity type definitions, but I don't quite get your description.. if you had a mismatch, shouldn't drush entup already have complained about that?

@Berdir possibly we've found an issue with this patch and I'm wondering if more people have ran into this.

We've updated a site from 8.4.8 to 8.7.2. After that we've started updating the modules, one of which is Paragraphs.

On update 8016 this call is being done: $entity_definition_update_manager->uninstallFieldStorageDefinition($storage_definition)

Eventually the call is going to check for data in the revision_uid field by calling $storage->countFieldData($storage_definition, TRUE)

Which is going to retrieve the table name for revision_uid, this is eventually done in a loop in core\lib\Drupal\Core\Entity\Sql\DefaultTableMapping.php:379 where it runs through multiple tables, the first being: paragraphs_item_field_data.

But instead of skipping this table, it falsely assumes revision_uid is in it because the columns being retrieved here are not the columns of the BaseTable anymore, but the columns retrieved by:

core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php:208
  $this->fieldStorageDefinitions = $this->entityFieldManager->getActiveFieldStorageDefinitions($entity_type->id());

In the end update 8016 will fail with the following error:

`
SQLSTATE[42S22]: Column not found: 1054 Unknown column 'revision_uid' in 'where clause': SELECT 1 AS      [error]
expression
FROM
{paragraphs_item_field_data} t
WHERE revision_uid IS NOT NULL
LIMIT 1 OFFSET 0; Array
(
)
` 

I was doubting to post this on Paragraphs, but as far as I can tell now this is a backwards compatibility issue in core, and not so much in Paragraphs. Do you have any thoughts on this?

Not quite sure if this is exactly the same issue but i recognized some issues on 8.6.16 with menu_link_content module (core):

General error: 1364 Field 'revision_id' doesn't have a default value: INSERT INTO {menu_link_content_data} (id, bundle, langcode, enabled, title, description, menu_name, link__uri, link__title, link__options, external, rediscover, weight, expanded, parent, changed, default_langcode, content_translation_source, content_translation_outdated, content_translation_uid, content_translation_status, content_translation_created) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3, :db_insert_placeholder_4, :db_insert_placeholder_5, :db_insert_placeholder_6, :db_insert_placeholder_7, :db_insert_placeholder_8, :db_insert_placeholder_9, :db_insert_placeholder_10, :db_insert_placeholder_11, :db_insert_placeholder_12, :db_insert_placeholder_13, :db_insert_placeholder_14, :db_insert_placeholder_15, :db_insert_placeholder_16, :db_insert_placeholder_17, :db_insert_placeholder_18, :db_insert_placeholder_19, :db_insert_placeholder_20, :db_insert_placeholder_21), (:db_insert_placeholder_22, :db_insert_placeholder_23, :db_insert_placeholder_24, :db_insert_placeholder_25, :db_insert_placeholder_26, :db_insert_placeholder_27, :db_insert_placeholder_28, :db_insert_placeholder_29, :db_insert_placeholder_30, :db_insert_placeholder_31, :db_insert_placeholder_32, :db_insert_placeholder_33, :db_insert_placeholder_34, :db_insert_placeholder_35, :db_insert_placeholder_36, :db_insert_placeholder_37, :db_insert_placeholder_38, :db_insert_placeholder_39, :db_insert_placeholder_40, :db_insert_placeholder_41, :db_insert_placeholder_42, :db_insert_placeholder_43); Array ( [:db_insert_placeholder_0] => 99 [:db_insert_placeholder_1] => menu_link_content [:db_insert_placeholder_2] => de [:db_insert_placeholder_3] => 1 [:db_insert_placeholder_4] => Support-Portal Login [:db_insert_placeholder_5] => [:db_insert_placeholder_6] => main [:db_insert_placeholder_7] => https://servicedesk.indevis.de/my.policy [:db_insert_placeholder_8] => [:db_insert_placeholder_9] => a:0:{} [:db_insert_placeholder_10] => 0 [:db_insert_placeholder_11] => 0 [:db_insert_placeholder_12] => -48 [:db_insert_placeholder_13] => 0 [:db_insert_placeholder_14] => menu_link_content:2e80355c-12c4-4831-b3f4-857af1c91b82 [:db_insert_placeholder_15] => 1560328484 [:db_insert_placeholder_16] => 1 [:db_insert_placeholder_17] => und [:db_insert_placeholder_18] => 0 [:db_insert_placeholder_19] => 1 [:db_insert_placeholder_20] => 1 [:db_insert_placeholder_21] => 1548745975 [:db_insert_placeholder_22] => 99 [:db_insert_placeholder_23] => menu_link_content [:db_insert_placeholder_24] => en [:db_insert_placeholder_25] => 1 [:db_insert_placeholder_26] => Support Portal Login [:db_insert_placeholder_27] => [:db_insert_placeholder_28] => main [:db_insert_placeholder_29] => https://servicedesk.indevis.de/my.policy [:db_insert_placeholder_30] => [:db_insert_placeholder_31] => a:0:{} [:db_insert_placeholder_32] => 0 [:db_insert_placeholder_33] => 0 [:db_insert_placeholder_34] => -48 [:db_insert_placeholder_35] => 0 [:db_insert_placeholder_36] => menu_link_content:2e80355c-12c4-4831-b3f4-857af1c91b82 [:db_insert_placeholder_37] => 1560328484 [:db_insert_placeholder_38] => 0 [:db_insert_placeholder_39] => de [:db_insert_placeholder_40] => 0 [:db_insert_placeholder_41] => 1 [:db_insert_placeholder_42] => 1 [:db_insert_placeholder_43] => 1548747539 ) in Drupal\Core\Entity\Sql\SqlContentEntityStorage->saveToSharedTables() (Zeile 938 in /var/www/indevis-website/web/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php).

I just read in the duplicate that 8.6.16 not supposed to have revision_id for menu_link_content at all, but its still there after we did the upgrade to 8.6.16. Not entirely sure if there will be any other issues (or this resolved at all) when upgrading beyond 8.7x.

Best

I also run into an issue with active definitions for entity types.
The modules altering of the entity type info cache definitions did no longer work, see #3081143: Drupal Core 8.7 compability

IMO hook_entity_type_alter is no longer working in "some" circumstances ?!