EntityType::setStorageClass() violates its interface

Here is the patch. Please review and commit.

I'll try to work on this.

Here I have added a simple test method for the `setStorageClass` method.

Hope everything is in order.

+++ b/core/tests/Drupal/Tests/Core/Entity/EntityTypeTest.php
@@ -166,6 +166,20 @@ public function testGetStorageClass() {
+    $updated_entity_type = $entity_type->setStorageClass($controller . 'NewTest');
+    $this->assertSame($controller . 'NewTest', $updated_entity_type->handlers['storage']);

The issue here is that setStorageClass is now returning a full entityType object, not just the name of the storage class itself.

Also, since handlers is a private method of entityType, you can't call it here. So we have to use getStorageClass (which is getHanderClass('storage')). However, getHandlerClass has a check to make sure the class is defined, we can't test with just concatenating "NewTest" on the name of the class because then getHandlerClass returns null.

You can, however, set up the entity with a null storage class and then change it to the $controller class.

Just FYI, looks like this failure is not specifically in this new patch, but due to #2749955: Random fails in UpdatePathTestBase tests. Will hold off on retesting until that is resolved.

Requeueing the test in #9 as the random failures appear to have been accounted for.

Looking good - comments follow

+++ b/core/tests/Drupal/Tests/Core/Entity/EntityTypeTest.php
@@ -166,6 +166,16 @@ public function testGetStorageClass() {
+    $entity_type->setStorageClass($controller);
+    $this->assertSame($controller, $entity_type->getHandlerClass('storage'));

You need to test what you're actually fixing.

$this->assertSame($entity_type, $entity_type->setStorageClass($controller));

Right. New test does check to make sure that what we're getting back is $this.

Do we need any kind of testing that setStorageClass is actually setting a storage class? That was more what the original test was attempting, but not what this issue was fixing.

Do we need any kind of testing that setStorageClass is actually setting a storage class?

That seems to be covered by EntityTypeTest::testGetStorageClass().

Looks good to go.

Committed 24d528d and pushed to 8.3.x. Thanks!

Well, this changes the return value of the implementation (to match the interface), so it does have the teeniest bit of potential disruption.

However, it's very theoretical and unlikely that something could possibly rely on this implementation returning NULL, so I think it is fine to backport while we are still in beta. (I would not during RC or in a patch release, since EntityType is an important base class.)

Thanks @tstoeckler!