Change SqlContentEntityStorageSchema::requiresEntityDataMigration() to ask the old storage handler if it has data rather than assuming yes unless NULL storage

I ran into this bug while building Multiversion. Here's some provisional code that at least got it working. But a few things still needs work:

1. The patch currently assumes that the old storage handler is using SQL and using $this->isTableEmpty() to check if it has data. Best thing is probably to add hasData() to the storage hander, as suggested in #2333113-45: Add an EntityDefinitionUpdateManager so that entity handlers can respond (e.g., by updating db schema) to code updates in a controlled way (e.g., from update.php)
2. Patch also needs tests

I would actually call this a bug. Because right now it's simply not possible to swap storage controller for an entity type.

Added hasData() to the storage handler.

This should do it. We can't obviously ask the new storage handler before it's created...

FieldableEntityStorageInterface was refactored into DynamicallyFieldableEntityStorageInterface. I will do a reroll.

Rerolled the patch.

Whoops patch file was empty... Here it is again!

Patch work good. I believe only missing part is having test.

Patch needs comments.

Added comments.

Looks good and working fine.

+++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
@@ -178,23 +178,16 @@ public function requiresFieldStorageSchemaChanges(FieldStorageDefinitionInterfac
+      $original_storage = $original_storage_class::createInstance(\Drupal::getContainer(), $entity_type);

This will break if the old class is no longer available in the system. We should assume TRUE in this case as there is no way to access existing data.

@plach thank you for the review. Here is updated patch.

Re-rolled patch and added coverage comment to the test that seems to have got lost.

Also bumping the priority of this issue since without this patch it's impossible to switch storage controller on an already installed site (even if there's no content for the particular entity type you're trying to change for). The only way to walk around this is to create a installation profile that includes your handler from scratch. A major DX fail in my opinion. We can do better than that :)

This bug is blocking a stable release of the Multiversion contrib module.

Patch does not need tests anymore. Removing tag.

+++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
@@ -178,23 +178,19 @@ public function requiresFieldStorageSchemaChanges(FieldStorageDefinitionInterfac
+      $original_storage = $original_storage_class::createInstance(\Drupal::getContainer(), $entity_type);

I'm really unsure about this, but I cannot think about a better solution, so passing the ball to a committer. Maybe they will have a suggestion. Moving back to RTBC.

I think we should be unit testing the conditions in public function requiresEntityDataMigration(). It does not look like we have test coverage here.

The patch attached delegates responsibility for creating handler instances to the EntityManager.

Whilst reviewing this patch I found myself wondering if we should have an abstract base class for SqlContentEntityStorageSchema since it looks unlikely that a non SQL version of this class would need to have a different method for requiresEntityDataMigration() or requiresFieldDataMigration()

This looks way better to me, if we can get a new patch I am happy to RTBC it again once Alex's request for additional test coverage is addressed.

+1 on the abstract base class for the storage schema.

+1 on the abstract base class for the storage schema.

Let's do this in another issue - not necessary for this one :)

And forgot to upload a patch :)

it looks unlikely that a non SQL version of [SqlContentEntityStorageSchema] would need (...)

Well, a ContentEntityStorageSchema is an SQL-only concept anyway ?
(which I guess means +1 on the abstract class ?)

As of #2278017-79: When a content entity type providing module is uninstalled, the entities are not fully deleted, leaving broken reference, this is now a blocker for that issue. Since that's a critical, bumping this to critical too.

Added beta evaluation.

Tests were added in #22.

That brings it down to nitpicks:

1. +++ b/core/lib/Drupal/Core/Entity/EntityManagerInterface.php
   @@ -252,6 +252,23 @@ public function hasHandler($entity_type, $handler_type);
   +   * preferred since this has additional checking that the class exists and
   

   s/this/that/, on a first read I thought the 'this' was referring to *this* method, but it's referring to the *other* method, hence 'that' would be better.

2. +++ b/core/lib/Drupal/Core/Entity/EntityManagerInterface.php
   @@ -252,6 +252,23 @@ public function hasHandler($entity_type, $handler_type);
   +   * @param $class
   

   mixed typehint?

3. +++ b/core/lib/Drupal/Core/Entity/EntityManagerInterface.php
   @@ -252,6 +252,23 @@ public function hasHandler($entity_type, $handler_type);
   +   *   A handler instance
   

   Missing trailing period.

4. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
   @@ -178,23 +178,19 @@ public function requiresFieldStorageSchemaChanges(FieldStorageDefinitionInterfac
   +    // If the original storage class is different, then there might be
   +    // existing entities in that storage even if the new storage's base
   +    // table is empty.
   

   80 col rule.

5. +++ b/core/tests/Drupal/Tests/Core/Entity/Sql/SqlContentEntityStorageTest.php
   @@ -1181,6 +1181,71 @@ public function testLoadMultiplePersistentCacheMiss() {
   +    $select_fields->expects($this->once())
   +      ->method('countQuery')
   

   $select_count_query would be more consistent with the other variable names.

This was RTBC at #19. It was only un-RTBC'd for test coverage. The test coverage was added in #22. I reviewed the test coverage, and only found nitpicks. Fixing them myself to accelerate this issue, and hence unblock #2278017: When a content entity type providing module is uninstalled, the entities are not fully deleted, leaving broken reference.

The only remaining remark is that hasData() could be implemented in an abstract base class, but that's out of scope here and wouldn't be an API change or addition.

Hence: back to RTBC!

Actually tests weren't added in #22... we need to add test coverage of the requiresEntityDataMigration method.

The interdiff for #22 was in #20... just keeping everyone on their toes :)

Oh :( Pre-existing missing test coverage, blargh!

Sorry!

adding blocker tag, since this blocks #2278017: When a content entity type providing module is uninstalled, the entities are not fully deleted, leaving broken reference, so that the d8 blockers is accurate. Remove the blocker tag when this issue is fixed, and unpostpone 2278017.

added the issue summary template to the summary, but it still needs someone familiar with the issue to update the remaining tasks. And put an actual problem description and proposed resolution in the summary.

In figuring out how to write those tests, looked at #2333113: Add an EntityDefinitionUpdateManager so that entity handlers can respond (e.g., by updating db schema) to code updates in a controlled way (e.g., from update.php), where requiresEntityDataMigration() was introduced. At the time, it apparently wasn't possible to write tests until some other issues had landed.

This is completely over my head, but I tried to push it forward.

+++ b/core/tests/Drupal/Tests/Core/Entity/Sql/SqlContentEntityStorageSchemaTest.php
@@ -14,7 +14,6 @@
- * @group Entity

Oops!

Well, no matter, because this will likely need a reroll anyway to improve the test coverage I wrote.

Here's a re-rolled patch fixing all issues raised in #31 and #32.

I also updated issue summary to reflect the current state and added a draft change record since we're making additions to the entity storage interface: https://www.drupal.org/node/2387757

Maybe the phpdoc for EMI:: createHandlerInstance() should point that getHandler() is the preferred way to get the "official" handler for an entity type ?

#33: That all looks splendid!

#35: That's already there:

+++ b/core/lib/Drupal/Core/Entity/EntityManagerInterface.php
@@ -252,6 +252,23 @@ public function hasHandler($entity_type, $handler_type);
+   * Usually \Drupal\Core\Entity\EntityManagerInterface::getHandler() is
+   * preferred since that method has additional checking that the class exists
+   * and has static caches.

AFAICT this is now RTBC-worthy :)

Gah, sorry, patch reviewing on phone, don't mind me :-/

Aucun problème! :)

P.S.: If you find a good website or native app for reading/reviewing patches on phones/tablets, let me know. I tweeted about that a few months ago, no good suggestions. I guess we're just weird like that…

+++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
@@ -1765,6 +1765,17 @@ public function countFieldData($storage_definition, $as_bool = FALSE) {
+      ->countQuery()

->range(0,1) would be better with large data sets.

Sure.

Discovered #2388765: Improve performance of SqlContentEntityStorage::countFieldData() for large datasets when getting the result as a boolean whilst doing #40

Fixed the unit test.

Why don't we add an entity query to ContentEntityStorageBase ?

@chx good point. Easily fixed.

Oh we have getQuery? That's handy.

So we should always be using the original storage definition to work out if we have data if we can.

Is the first hunk intended? We discussed it but is it necessary? I somewhat doubt.

Yep it is 100% necessary - we need to use the original definition or we get the error seen in #45

Comments nitpick on the last interdiff :

1. +++ b/core/lib/Drupal/Core/Entity/EntityStorageBase.php
   @@ -460,7 +460,7 @@ public function loadByProperties(array $values = array()) {
   -    return \Drupal::entityQuery($this->getEntityTypeId(), $conjunction);
   +    return \Drupal::service($this->getQueryServicename())->get($this->entityType, $conjunction);
   

   If something non-trivial is happening here (cf discussion #49 / #50), does it warrant a comment ?

2. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
   @@ -179,17 +179,11 @@ public function requiresFieldStorageSchemaChanges(FieldStorageDefinitionInterfac
   -
   -    // If the original storage class is different, then there might be existing
   -    // entities in that storage even if the new storage's base table is empty.
   -    if ($entity_type->getStorageClass() != $original_storage_class) {
   -      if (!class_exists($original_storage_class)) {
   -        return TRUE;
   -      }
   -      $original_storage = $this->entityManager->createHandlerInstance($original_storage_class, $original);
   -      return $original_storage->hasData();
   +    if (!class_exists($original_storage_class)) {
   +      return TRUE;
        }
   -    return $this->storage->hasData();
   +    $original_storage = $this->entityManager->createHandlerInstance($original_storage_class, $original);
   +    return $original_storage->hasData();
      }
   

   Same here, we lose some comment ? Isn't this what this all issue here is about ?

@yched sure no problem.

@chx gave some feedback in IRC.

alexpott: then we need to comment on the QueryFactory not to override the service.

Whoever is coding the removal this change here makes it easier to understand what's going on.

So #40 + #43 was to change from a COUNT query to a (0, 1) range query. This is fine because we don't care how many entities there are, only if there are any entities at all.

#45 only moves the existing code to a different class (a base class).

#48 is the only significant change. Instead of either checking the original storage or the new storage, we now only check the original storage. Thinking about this, it makes sense to me, because how could the new storage class already contain any data, if what we're doing is updating the entity type definition (i.e. schema or storage class changes)? But I wonder how we missed this so far, and why we didn't notice it sooner?

I don't understand what's going on in #53/#54, but perhaps that's because I'm tired.

1. +++ b/core/tests/Drupal/Tests/Core/Entity/Sql/SqlContentEntityStorageTest.php
   @@ -1202,10 +1202,10 @@ public function testHasData() {
   -      ->with('entity_test', 'AND')
   +      ->with($this->entityType, 'AND')
   

   This test is now effectively dead, because $this->entityType is a mock object, not the string 'entity_test'. This should have been $this->entityType->id(). Yet the test still passes.

   (Verified by debugging locally.)

   NW for this.

2. +++ b/core/lib/Drupal/Core/Entity/EntityStorageBase.php
   @@ -460,6 +460,8 @@ public function loadByProperties(array $values = array()) {
   +    // Access the service directly rather than entity.query factory so the
   +    // storage's current entity type is used.
   

   This sounds a bit like dark magic to me, but perhaps that's because I don't know this well enough.

re #56

1. We've changed from mocking entity.query to mocking entity.query.sql - these have different arguments.
2. This is actually less dark magic than getting the entity.query service and then getting the storage to get the query service name and entity type definition :)

1. +++ b/core/lib/Drupal/Core/Entity/Query/QueryFactory.php
   @@ -13,8 +13,17 @@
   + * This class is marked as final since it should not be overridden in the
   + * container. EntityStorageBase::getQuery() accesses the storages query service
   + * directly without using this factory.
   

   ... Mh, so it should not be overridden, but why? For example the webprofiler module extends a hell lot of services you might never have thought to be overridden.

2. +++ b/core/lib/Drupal/Core/Entity/Query/QueryFactory.php
   @@ -13,8 +13,17 @@
   +final class QueryFactory implements ContainerAwareInterface {
   

   -1e5 ... that makes mocking in unit tests impossible.

3. +++ b/core/tests/Drupal/Tests/Core/Entity/Sql/SqlContentEntityStorageTest.php
   @@ -1181,6 +1181,56 @@ public function testLoadMultiplePersistentCacheMiss() {
   +    $factory = $this->getMockBuilder('Drupal\Core\Entity\Query\QueryFactory')
   +      ->disableOriginalConstructor()
   +      ->getMock();
   +    $factory->expects($this->once())
   +      ->method('get')
   +      ->with($this->entityType, 'AND')
   +      ->willReturn($query);
   

   HAHA, even this patch will fail, because of the final change.

re #58

1. so the patch tells you why.
2. lol yes - fixed
3. lol some more

Correct interdiff for #59

I did not read any of the existing discussion, only have some documentation suggestions/nitpicks, ignore if it was already discussed. Implementation looks good to me.

1. +++ b/core/lib/Drupal/Core/Entity/DynamicallyFieldableEntityStorageInterface.php
   @@ -85,6 +85,13 @@ public function purgeFieldData(FieldDefinitionInterface $field_definition, $batc
   +   * @return bool
   +   */
   

   Nitpick, but afaik we always need a description for @return except when it is $this. But I could be wrong.

2. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
   @@ -324,19 +324,7 @@ public function getHandler($entity_type, $handler_type) {
   -      if (is_subclass_of($class, 'Drupal\Core\Entity\EntityHandlerInterface')) {
   -        $handler = $class::createInstance($this->container, $definition);
   -      }
   -      else {
   -        $handler = new $class($definition);
   -      }
   -      if (method_exists($handler, 'setModuleHandler')) {
   -        $handler->setModuleHandler($this->moduleHandler);
   -      }
   -      if (method_exists($handler, 'setStringTranslation')) {
   -        $handler->setStringTranslation($this->translationManager);
   -      }
   -      $this->handlers[$handler_type][$entity_type] = $handler;
   +      $this->handlers[$handler_type][$entity_type] = $this->createHandlerInstance($class, $definition);
   

   Wondering if we can use the new method also in #2350503: Add route generation handlers for entities ?

3. +++ b/core/lib/Drupal/Core/Entity/EntityManagerInterface.php
   @@ -252,6 +252,23 @@ public function hasHandler($entity_type, $handler_type);
   +   * @return mixed
   +   *   A handler instance.
   

   Would @return object be better here?

4. +++ b/core/lib/Drupal/Core/Entity/Query/QueryFactory.php
   @@ -13,6 +13,15 @@
   + * This class should not be overridden in the container.
   + * EntityStorageBase::getQuery() accesses the storages query service directly
   + * without using this factory.
   

   Hm. I don't think we care if it is overridden or not, as long as the implementation doesn't change?

   As @dawehner mentioned, use cases like wrapping the original call to do statistics or similar things would be perfectly valid?

   So why not just document something like "Any implementation of this service must call getQuery() of the corresponding entity storage." ?

5. +++ b/core/tests/Drupal/Tests/Core/Entity/Sql/SqlContentEntityStorageSchemaTest.php
   @@ -1056,6 +1056,85 @@ public function testDedicatedTableSchemaForEntityWithStringIdentifier() {
   +      ->willReturn('\Drupal\Core\Entity\Sql\SqlContentEntityStorageSchema'); // A class that exists, *any* class.
   

   Not sure if there is a reason that those comments are appended like this, doesn't really follow the coding standard :)

1. Fixed
2. Yes you are right
3. Fixed - you are right this is better - also changing getHandler() to be consistent
4. Fixed - this makes QueryFactory::getAggregate() inconsistent. The patch attached fixes this and protects getQueryServiceName which makes it much harder to create an implementation of QueryFactory that does not call the getQuery / getAggregateQuery on the entity storages
5. Fixed

Great, RTBC if green I'd say.

The only thing you didn't update and I'm not sure if/how is the comment in core.services.yml...

@Berdir right you are

+++ b/core/lib/Drupal/Core/Entity/EntityStorageBase.php
@@ -460,7 +460,26 @@ public function loadByProperties(array $values = array()) {
+    return \Drupal::service($this->getQueryServicename())->get($this->entityType, $conjunction);

This should be getQueryServiceName().

@tstoeckler - right you are.

Looked through this a couple of times and couldn't find anything to complain about. Committed/pushed to 8.0.x, thanks!