Problem/Motivation

Follow-up from #2278017: When a content entity type providing module is uninstalled, the entities are not fully deleted, leaving broken reference: ContentUninstallValidator relies on ContentEntityStorage::hasData() but this currently resides in DynamicallyFieldableEntityStorageInterface, which is not required for content entities to implement.

Proposed resolution

As it makes sense generically, let's move it up to EntityStorageInterface. The implementation should work for config entities as well, but this needs tests.

Remaining tasks

Do.

User interface changes

-

API changes

- EntityStorageInterface::hasData() is added (moved up from DynamicallyFieldableEntityStorageInterface)

CommentFileSizeAuthor
#22 2391829-22.patch3.68 KBcilefen
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,112 pass(es), 173 fail(s), and 0 exception(s). View
#21 2391829-21.patch0 bytescilefen
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,251 pass(es). View
#16 has-data-2391829-16.patch3.66 KBBerdir
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,231 pass(es), 2 fail(s), and 0 exception(s). View
#16 has-data-2391829-16-test-only.patch1019 bytesBerdir
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,983 pass(es), 2 fail(s), and 0 exception(s). View
#4 2391829-4.patch2.64 KBcilefen
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,446 pass(es). View
#4 interdiff-2-4.txt464 bytescilefen
#2 2391829-2.patch2.04 KBcilefen
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

fago’s picture

Status: Postponed » Active
cilefen’s picture

Status: Active » Needs review
FileSize
2.04 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

Status: Needs review » Needs work

The last submitted patch, 2: 2391829-2.patch, failed testing.

cilefen’s picture

Status: Needs work » Needs review
FileSize
464 bytes
2.64 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,446 pass(es). View
plach’s picture

Way better :)

Can we add an implementation for ContentEntityNullStorage?

cilefen’s picture

@plach There already is one without this patch.

plach’s picture

Status: Needs review » Reviewed & tested by the community

Mmh, I should really get some rest. This looks ready to me :)

plach’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/lib/Drupal/Core/Entity/KeyValueStore/KeyValueEntityStorage.php
@@ -202,6 +202,13 @@ protected function has($id, EntityInterface $entity) {
+    return FALSE;

Would it make sense to return this?

(bool) count($this->keyValueStore->getAll())

I'm wondering whether it could imply performance problems.

Berdir’s picture

Possibly. The query implementation also uses getAll(), there is nothing else that we could use.

It's just a test implementation really, I hope nobody is going to use it for real data :)

yched’s picture

Status: Needs review » Reviewed & tested by the community

Yay - looks good indeed ?

fago’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php
@@ -279,6 +279,13 @@ protected function has($id, EntityInterface $entity) {
+  public function hasData() {
+    return FALSE;
+  }

Why that? It should just run an entity query as for content entities + we need test coverage that hasData() works for for config entities.

yched’s picture

As a side note : #2392351: When an entity bundle config gets deleted, entities of that bundle break has a case for "are there existing entities *for that specific bundle* ?", and curently has to include dedicated code that almost duplicates ContentEntityStorage::hasData(), just adding an extra ->condition($entity_type->getKey('bundle'), $bundle)

Would be a nice thing to add as an optional param in ContentEntityStorage::hasData() ?

yched’s picture

And right, having the implemtation always returning FALSE for ConfigEntityStorage & KeyValueEntityStorage is a bit problematic.

- KeyValueEntityStorage would need to return count($this->keyValueStore->getAll()) != 0
Although, yeah, as mentioned above, that is kind of a perf drag :-/

From @Berdir #9:

KeyValueEntityStorage just a test implementation really, I hope nobody is going to use it for real data :)

OK, but nothing really hints that it's a test-only-do-not-use-for-real implementation :-).
It's not in a "test" directory or namespace, Drupal\Core\Entity\KeyValueStore makes it kind of "officially endorsed", and no comment or phpdoc say anything about that either.

So maybe the we should go with the getAll() approach for now, and open a separate issue to move that class to a test folder somewhere ?

- ConfigEntityStorage would need to return count($this->configFactory->listAll()) != 0
That's not as bad as KV::getAll(), since this only load keys into memory. Could still be 100's of entries just to answer "is there at least one ?". Making that more efficient means adding a new method to ConfigFactoryInterface & Config\StorageInterface, though.

Berdir’s picture

OK, but nothing really hints that it's a test-only-do-not-use-for-real
implementation :-).
It's not in a "test" directory or namespace, Drupal\Core\Entity\KeyValueStore
makes it kind of "officially endorsed", and no comment or phpdoc say anything
about that either.

So maybe the we should go with the getAll() approach for now, and open a
separate issue to move that class to a test folder somewhere ?

/me points to @timplunkett ;)

I know and I'd be happy to do that.

yched’s picture

Berdir’s picture

FileSize
1019 bytes
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,983 pass(es), 2 fail(s), and 0 exception(s). View
3.66 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,231 pass(es), 2 fail(s), and 0 exception(s). View

Ok, here's a patch with implementation for config and keyvalue. I also added a test, this also runs for key value storage.

plach’s picture

Status: Needs work » Needs review
plach’s picture

Issue tags: +entity storage

The last submitted patch, 16: has-data-2391829-16-test-only.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 16: has-data-2391829-16.patch, failed testing.

cilefen’s picture

Status: Needs work » Needs review
FileSize
0 bytes
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,251 pass(es). View

reroll

cilefen’s picture

FileSize
3.68 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,112 pass(es), 173 fail(s), and 0 exception(s). View

oops

The last submitted patch, 21: 2391829-21.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 22: 2391829-22.patch, failed testing.

cilefen’s picture

+++ b/core/modules/config/src/Tests/ConfigEntityTest.php
@@ -39,6 +39,10 @@ class ConfigEntityTest extends WebTestBase {
+
+    $storage = \Drupal::entityManager()->getStorage('config_test');
+    $this->assertFalse($storage->hasData());

I think this test is failing because the config_test module sets some configurations objects on install.

The last submitted patch, 22: 2391829-22.patch, failed testing.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.