ContentUninstallValidator relies on the not required ContentEntityStorage::hasData() method

Way better :)

Can we add an implementation for ContentEntityNullStorage?

@plach There already is one without this patch.

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

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

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

Yay - looks good indeed ?

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

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.

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.

Opened #2393751: Document that KeyValueEntityStorage is not scalable beyond a few hundred entities :-)

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

reroll

oops

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

#25 is correct, so we simply need to move those assertions to a test that doesn't install config_test entities by default. And, since KeyValueEntityStorage is not used as a test config storage anymore, we need to add a dedicated test for that as well.

And to show why this is still a major bug, I've added a simple test that tries to see if uninstalling a module which provides an entity type that doesn't use the SQL content entity storage is possible.

This simulates a user navigating to the module uninstall page and being presented with a nice fatal error:

Error: Call to undefined method Drupal\Core\Entity\KeyValueStore\KeyValueContentEntityStorage::hasData()

The test-only patch is also the interdiff.

Patch looks good to me, but not sure if adding a method to EntityStorageInterface now is a BC break, since there is no default implemtentation then it would break alternative entity storage implementations. So possibly we need a separate interface now, with a @todo to merge it into the base interface in 9.x?

double post because of 5xx error

@Berdir, isn't \Drupal\Core\Entity\EntityStorageBase the default implementation? We could move \Drupal\Core\Entity\ContentEntityStorageBase::hasData() to EntityStorageBase because that implementation is generic and it doesn't depend on anything else but the entity query, which is initialized in EntityStorageBase anyway.

Yes, that makes sense, lets do that :)

Done that ;)

Great, I think this is OK now for BC. We might still want to do a short change record?

Sure thing, here it is: https://www.drupal.org/node/2907014

Committed/pushed to 8.5.x and cherry-picked to 8.4.x. Thanks! Publishing the change record.