Problem/Motivation

Followup from #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).18.
Blocks #2278017: When a content entity type providing module is uninstalled, the entities are not fully deleted, leaving broken reference.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue priority Critical because this blocks #2278017: When a content entity type providing module is uninstalled, the entities are not fully deleted, leaving broken reference, which is a critical upgrade path issue.
Disruption Zero disruption.

Proposed resolution

  1. Add ::hasData() to the entity storage interface and create implementations for it in the NULL handler and the default SQL handler
  2. Change SqlContentEntityStorageSchema::requiresEntityDataMigration() to use ::hasData to check if a migration is required.

Remaining tasks

Contributor tasks needed
Task Novice task? Contributor instructions Complete?
Review patch to ensure that it fixes the issue, stays within scope, is properly documented, and follows coding standards Instructions
When this is fixed, unpostpone #2278017: When a content entity type providing module is uninstalled, the entities are not fully deleted, leaving broken reference

User interface changes

None

API changes

Adding ::hasData to the entity storage interface

Original report by @effulgentsia

Files: 
CommentFileSizeAuthor
#68 2335879-2.68.patch21 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,173 pass(es). View
#68 66-68-interdiff.txt1.17 KBalexpott
#66 2335879-2.66.patch21 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,163 pass(es). View
#66 64-66-interdiff.txt449 bytesalexpott
#64 2335879-2.64.patch21.43 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,152 pass(es). View
#64 59-64-interdiff.txt9.63 KBalexpott
#60 54-59-interdiff.txt1.58 KBalexpott
#59 2335879-2.59.patch16.4 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,143 pass(es). View
#59 54-59-interdiff.txt802 bytesalexpott
#54 2335879_54.patch16.52 KBchx
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,089 pass(es), 27 fail(s), and 0 exception(s). View
#54 interdiff.txt802 byteschx
#53 2335879-2.53.patch15.99 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View
#53 52-53-interdiff.txt1.32 KBalexpott
#52 2335879-2.52.patch14.67 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,099 pass(es). View
#52 48-52-interdiff.txt1.69 KBalexpott
#48 2335879-2.48.patch14.32 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,152 pass(es). View
#48 45-48-interdiff.txt6.73 KBalexpott
#45 2335879-2.45.patch13.97 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,123 pass(es), 0 fail(s), and 1 exception(s). View
#45 43-45-interdiff.txt4.08 KBalexpott
#43 2335879-2.43.patch14.58 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,106 pass(es). View
#43 40-43-interdiff.txt1.33 KBalexpott
#40 2335879-2.40.patch14.59 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#40 33-40-interdiff.txt505 bytesalexpott
#33 interdiff.txt3.64 KBdixon_
#33 2335879-ask-old-storage-handler-33.patch14.59 KBdixon_
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,111 pass(es). View
#30 2335879-30.patch14.98 KBWim Leers
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,032 pass(es). View

Comments

dixon_’s picture

Status: Active » Needs work
FileSize
1.36 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,905 pass(es). View

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
dixon_’s picture

Category: Task » Bug report

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

dixon_’s picture

Status: Needs work » Needs review
FileSize
3.93 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,226 pass(es), 1 fail(s), and 1 exception(s). View

Added hasData() to the storage handler.

Status: Needs review » Needs work

The last submitted patch, 3: 2335879-ask-old-storage-handler-2.patch, failed testing.

dixon_’s picture

Status: Needs work » Needs review
FileSize
3.9 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,614 pass(es). View

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

mauzeh’s picture

Status: Needs review » Needs work
Issue tags: +Novice, +Amsterdam2014

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

mauzeh’s picture

Status: Needs work » Needs review
FileSize
0 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,767 pass(es). View

Rerolled the patch.

mauzeh’s picture

FileSize
3.94 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,997 pass(es). View

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

ygerasimov’s picture

Issue tags: +Needs tests

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

ygerasimov’s picture

FileSize
6.53 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,752 pass(es). View

Patch needs comments.

jeqq’s picture

FileSize
6.6 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,902 pass(es). View

Added comments.

gumanist’s picture

Status: Needs review » Reviewed & tested by the community

Looks good and working fine.

plach’s picture

Status: Reviewed & tested by the community » Needs work
+++ 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.

ygerasimov’s picture

Status: Needs work » Needs review
FileSize
6.61 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,766 pass(es). View

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

dixon_’s picture

Priority: Normal » Major
FileSize
6.63 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,914 pass(es). View
685 bytes

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 :)

dixon_’s picture

Issue tags: +contrib dependency

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

dixon_’s picture

dixon_’s picture

Issue tags: -Needs tests

Patch does not need tests anymore. Removing tag.

plach’s picture

Status: Needs review » Reviewed & tested by the community
+++ 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.

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: -Novice +Needs tests
FileSize
4.25 KB

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()

plach’s picture

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.

alexpott’s picture

FileSize
10.09 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,806 pass(es). View

+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 :)

yched’s picture

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 ?)

Wim Leers’s picture

Priority: Major » Critical
Issue summary: View changes
Status: Needs review » Needs work
Issue tags: -Needs tests +API addition

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.

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
10.62 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,014 pass(es). View
3.58 KB

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!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

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

alexpott’s picture

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

Wim Leers’s picture

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

Sorry!

YesCT’s picture

Issue summary: View changes
Issue tags: +blocker, +Needs issue summary update

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.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
14.98 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,032 pass(es). View
4.41 KB

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.

Wim Leers’s picture

+++ 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.

effulgentsia’s picture

Thanks for starting on the test. For more background to #30, #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) introduced requiresEntityDataMigration() and added an integration test (EntityDefinitionUpdateTest) for what uses it (SqlContentEntityStorageSchema::onEntityTypeUpdate()), but did not add a dedicated unit test for the new method itself. That was just an oversight. Glad that's being addressed here.

+++ b/core/tests/Drupal/Tests/Core/Entity/Sql/SqlContentEntityStorageSchemaTest.php
@@ -1056,6 +1055,78 @@ public function testDedicatedTableSchemaForEntityWithStringIdentifier() {
+      // Case 4: different storage class, original storage class exists, ::hasData() === TRUE.
+      [$updated_entity_type_definition, $original_entity_type_definition_other_existing, TRUE, TRUE],
+      // Case 5: different storage class, original storage class exists, ::hasData() === FALSE.
+      [$updated_entity_type_definition, $original_entity_type_definition_other_existing, FALSE, FALSE],
...
+    // Assert hasData() is not called when $storage_has_data === NULL.
+    $this->storage->expects($this->exactly(is_null($storage_has_data) ? 0 : 1))
+      ->method('hasData')
+      ->willReturn($storage_has_data);
...
+    $this->entityManager->expects($this->any())
+      ->method('createHandlerInstance')
+      ->willReturn($this->storage);
...
+    $this->storageSchema = $this->getMockBuilder('Drupal\Core\Entity\Sql\SqlContentEntityStorageSchema')
+      ->setConstructorArgs(array($this->entityManager, $this->entityType, $this->storage, $connection))

The test mostly looks good, but what's confusing about it is that it's dealing with a single mocked $this->storage object that in the case of the storage classes being different is still serving as the mock of each one. Therefore, we're not testing that hasData() is being invoked on the correct one (i.e., the one for the original class if it's different). So I think we want to expand cases 4 and 5 into 2 versions each (i.e., change the hasData parameter into 2 parameters: one for the new storage and one for the old).

However, unless someone fixes that quickly, I wonder if it makes sense to commit either #25 or #30 without waiting on that, in order to unblock #2278017: When a content entity type providing module is uninstalled, the entities are not fully deleted, leaving broken reference, and then continue the test refinements in parallel.

dixon_’s picture

Issue summary: View changes
Issue tags: -Needs tests, -Needs issue summary update
FileSize
14.59 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,111 pass(es). View
3.64 KB

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

dixon_’s picture

Issue summary: View changes
yched’s picture

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

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

#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 :)

yched’s picture

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

Wim Leers’s picture

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…

catch’s picture

+++ 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.

alexpott’s picture

FileSize
505 bytes
14.59 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

Sure.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 40: 2335879-2.40.patch, failed testing.

alexpott’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
1.33 KB
14.58 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,106 pass(es). View

Fixed the unit test.

chx’s picture

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

alexpott’s picture

FileSize
4.08 KB
13.97 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,123 pass(es), 0 fail(s), and 1 exception(s). View

@chx good point. Easily fixed.

chx’s picture

Oh we have getQuery? That's handy.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 45: 2335879-2.45.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
6.73 KB
14.32 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,152 pass(es). View

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

chx’s picture

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

alexpott’s picture

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

yched’s picture

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 ?

alexpott’s picture

FileSize
1.69 KB
14.67 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,099 pass(es). View

@yched sure no problem.

alexpott’s picture

FileSize
1.32 KB
15.99 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View

@chx gave some feedback in IRC.

alexpott: then we need to comment on the QueryFactory not to override the service.
chx’s picture

FileSize
802 bytes
16.52 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,089 pass(es), 27 fail(s), and 0 exception(s). View

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

The last submitted patch, 53: 2335879-2.53.patch, failed testing.

Wim Leers’s picture

Status: Needs review » Needs work

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.

alexpott’s picture

Status: Needs work » Needs review

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 :)
dawehner’s picture

  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.

alexpott’s picture

FileSize
802 bytes
16.4 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,143 pass(es). View

re #58

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

FileSize
1.58 KB

Correct interdiff for #59

effulgentsia’s picture

#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?

The pre-#48 patches checked the new storage only if it was the same class as the original storage. So under most circumstances that's fine. However, it is possible that something else about the entity type's storage is being changed without changing the class, so I agree that the post-#48 patches are better in that regard. And the assertion that the new storage's hasData() isn't called is sufficient test coverage of that, IMO.

The last submitted patch, 54: 2335879_54.patch, failed testing.

Berdir’s picture

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 :)

alexpott’s picture

FileSize
9.63 KB
21.43 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,152 pass(es). View
  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
Berdir’s picture

Status: Needs review » Reviewed & tested by the community

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...

alexpott’s picture

FileSize
449 bytes
21 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,163 pass(es). View

@Berdir right you are

tstoeckler’s picture

Status: Reviewed & tested by the community » Needs work
+++ 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().

alexpott’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
1.17 KB
21 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,173 pass(es). View

@tstoeckler - right you are.

catch’s picture

Status: Reviewed & tested by the community » Fixed

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

  • catch committed 8298978 on 8.0.x
    Issue #2335879 by alexpott, dixon_, Wim Leers, chx, mauzeh, ygerasimov,...

Status: Fixed » Closed (fixed)

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