EntityType objects cannot be reliably serialized without DependencySerializationTrait

Critical priority is for bugs that are actively causing data loss or other catastrophic failures. This is certainly a big issue, but it's not critical :) I'll look into it.

Congratulations, @a.dmitriiev -- you've turned up a bug in Drupal core!

I am updating the IS with the appropriate information and attaching a failing test which proves the problem.

Bit of cleanup.

I said this was a major issue, and yet I left it with normal priority. Derp.

Here is an improved (failing) test to illustrate what I think the real problem is here -- namely, the god-awful DX of implicitly creating an unserializable dependency, which happens because StringTranslationTrait::getStringTranslation() sets $this->stringTranslation if it's not set already.

IMHO, the correct fix here -- even though it might constitute an API break -- is that getStringTranslation() should never set $this->stringTranslation. It should use it if it's there, but no more than that.

This approach will not prevent people from hitting the database serialization exception if setStringTranslation() has been called, but that's OK, I think -- the point is that it will no longer be possible to run into that condition by accident. The implicitness is what makes this such an insidious land mine, and we need to kill it dead. With fire.

EDIT: Also, it is not good practice for a getter to be setting things. Such a thing would be unacceptable in a core patch, so why are we OK with it being done by a trait that is used by half of core??

Discussed a bit in IRC. I initially suggested to copy the __sleep() but that actually also doesn't work as you can't inherit the same method from two traits, fails in a different way.

I'm +1 on not doing the set anymore in getStringTranslation(), probably with a @todo to use DST instead/anyway in 9.x, to also help with those that explicitly set the value.

Those that do that will likely be in a class that actually does use DST, e.g. a controller or form.

Okay -- this oughta do the trick.

Re-titling for clarity.

We'll need a change record, as per this point @Berdir raised on IRC:

<phenaproxima> berdir: Do you think that will need a change record?
<berdir> phenaproxima: probably. the problem is that I'm sure there is code out there that does $this->stringTranslation->t() after initially calling $this->t() or getStringTranslation() earlier. and that would break now. which I guess is OK, bcause it's not an API, but it would likely still be helpful if they end up searching for the error

Yes, there is: apply the patch in #9 and you should be OK.

Change record added: https://www.drupal.org/node/2893876.

What confuses me, don't you have the same problem when you have something actually injecting the service, which is sort of the recommendation?

Yes. If you explicitly inject the service without DependencySerializationTrait, you will encounter the exception during serialization (if locale is enabled, of course).

As I said in #7, though: without this patch, that condition could happen essentially by accident if you, or anything you are referencing (explicitly or implicitly) uses StringTranslationTrait without DependencySerializationTrait. With this patch, the only way you will encounter the serialization exception is if you (or a parent class you are extending) have explicitly injected the service. Which means you are in a position to actually fix the problem, either by removing the reference to the service, or by importing DependencySerializationTrait.

That is why this is tagged as a DX improvement. The point isn't to prevent the exception entirely -- just to prevent it from happening by accident.

@phenaproxima
Please put these kind of information into the issue summary, that's where they are really helpful :)

The test change from #3 to #7 (no interdiff!) hides the actual bug as discovered: \Drupal\Core\Entity\EntityType needs to use DependencySerializationTrait.
Instead of fixing that, this slightly mitigates the landmine of serialization by preventing it in the implicit case but not the explicit case of string translation. Furthermore, this is a pretty needless BC break, and removes a micro-optimization.

My recommendation in IRC was to introduce a new version of STT that incorporates DST. It cannot be added to the existing trait due to PHP's collision handling logic. That part of the conversation didn't make it to the issue.

TL;DR why not fix the bug instead of hiding it deeper?

Clarifying the IS a bit.

Instead of fixing that, this slightly mitigates the landmine of serialization by preventing it in the implicit case but not the explicit case of string translation.

In my view, this is acceptable from a DX standpoint. Because, again, with the way things currently are, you may have no way to fix the problem if you encounter it. With the proposed fix, you do. That makes a world of difference.

StringTranslationTrait::getStringTranslation() is a protected non-API method, so as far as I know, this is an acceptable change under the BC policy. And yes, it removes a micro-optimization...that causes a bug. Why would we want to keep weird, buggy, sometimes-unfixable behavior in the name of micro-optimization?

I'm not against having a new trait (I imagine it could be called SerializableStringTranslationTrait), but that is a messier solution, in my opinion, and introduces more BC concerns down the line.

Opened #2893917: Create a version of StringTranslationTrait that uses DependencySerializationTrait as the follow-up.
Here's the fix from #9 but with the test from #3 (which is more correct).
And here's the correct short-term fix, until the above issue is resolved.

Adding #2893917: Create a version of StringTranslationTrait that uses DependencySerializationTrait as a related issue.

Tested the patch with the latest lightning release and it works fine. Also created this followup to get the patch into lightning: #2902229: Add patch for "StringTranslationTrait consumers cannot be reliably serialized"

I went through all classes using StringTranslationTrait, but don't have DependencySerializationTrait ...

• \Drupal\Core\Config\ConfigImportValidateEventSubscriberBase
• \Drupal\Core\Config\ConfigManager
• \Drupal\Core\Controller\ControllerBase
• \Drupal\Core\Controller\TitleResolver
• \Drupal\Core\Datetime\DateFormatter
• \Drupal\Core\Datetime\DrupalDateTime
• \Drupal\Core\Entity\ContentUninstallValidator
• \Drupal\Core\Entity\EntityConstraintViolationList
• \Drupal\Core\Entity\EntityDefinitionUpdateManager
• \Drupal\Core\Entity\EntityDisplayRepository
• \Drupal\Core\Entity\EntityFieldManager
• \Drupal\Core\Entity\EntityTypeRepository
• \Drupal\Core\Entity\Controller\EntityController
• \Drupal\Core\EventSubscriber\FinalExceptionSubscriber
• \Drupal\Core\EventSubscriber\MaintenanceModeSubscriber
• \Drupal\Core\Extension\RequiredModuleUninstallValidator
• \Drupal\Core\Field\FieldModuleUninstallValidator
• \Drupal\Core\Form\FormValidator
• \Drupal\Core\Form\EventSubscriber\FormAjaxSubscriber
• \Drupal\Core\Installer\Exception\InstallerException
• \Drupal\Core\Language\ContextProvider\CurrentLanguageContext
• \Drupal\Core\Mail\MailManager
• ...

Given that I'm wondering whether it would be just better to solve just #2902229: Add patch for "StringTranslationTrait consumers cannot be reliably serialized" and commit a patch there.

The patch 2893029-full-fix-test-from-3.patch fixed a fatal error for me during the upgrade to Drupal 8.5. This is quite possibly related to a contrib module, but I can imagine there are going to be more people having this issue. Is there a possibility to fix this before the 8.5 release?

#21 (2893029-full-fix-test-from-3.patch) doesn't solve my problem.
I still get:
LogicException: The database connection is not serializable. This probably means you are serializing an object that has an indirect reference to the database connection. Adjust your code so that is not necessary. Alternatively, look at DependencySerializationTrait as a temporary solution. in Drupal\Core\Database\Connection->__sleep() (line 1472 of /var/www/html/drupal8/core/lib/Drupal/Core/Database/Connection.php).

I get this error when clicking on the edit button of a paragraphs widget (when editing a node)

Changed status because of problem in #29

Just because it didn't fix your problem doesn't mean that this isn't working.

There are many possible causes for such a serialization error, a related issue that's basically the same as this with TypedDataTrait is #2986735: Drupal\Core\Plugin\Context\Context needs DependencySerializationTrait, so maybe that will help with your specific problem. But it could be anything, try tracking it down a specific widget/field type if my patch over there doesn't help here.

We have several confirmed reports that this is helping and there are few things more annoying and harder to debug & fix than those kind of serialization errors.

Needs a re-roll.

Rerolled against 8.7.x.

Restoring RTBC since this was a straight re-roll and I expect all backends to be green.

Quick review of this.

1. +++ b/core/modules/locale/tests/src/Kernel/EntityTypeSerializationTest.php
   @@ -0,0 +1,37 @@
   + * Tests that entity type definitions can be serialized when Locale is present.
   + *
   + * @group locale
   

   Is this really in the locale group/module? The 'string_translation' service is in core and so is StringTranslationTrait.

   Edit: re-read the issue summary, locale is required to be installed to reproduce this. Fine for this to stay here.

2. +++ b/core/modules/locale/tests/src/Kernel/EntityTypeSerializationTest.php
   @@ -0,0 +1,37 @@
   +    'field',
   +    'file',
   +    'language',
   

   The test passes/failes without any of these modules installed.

3. +++ b/core/modules/locale/tests/src/Kernel/EntityTypeSerializationTest.php
   @@ -0,0 +1,37 @@
   +
   +  public function testEntityTypeSerializationWithTranslationService() {
   

   Missing a docblock, not sure if that's intentional.

4. +++ b/core/modules/locale/tests/src/Kernel/EntityTypeSerializationTest.php
   @@ -0,0 +1,37 @@
   +    serialize($entity_type);
   

   Any reason we don't immediately also unserialize this in the test as well? Would be good to see the string translation operable after the unserialization too.

Rethinking #36.1, this is really a bug in the EntityType class, it's only exposed by locale, but would be triggered with any non-serializeable string_translation service. For that reason, we can move the test to a more appropriate place in core and decouple it from locale entirely with a unit test:

  public function testEntityTypeSerializationWithTranslationService() {
    $entity_type = new EntityType([
      'id' => 'foo',
    ]);
    $translation = $this->prophesize(TranslationInterface::class);
    $translation->willImplement(\Serializable::class);
    $translation->serialize()->willThrow(\Exception::class);
    $translation_service = $translation->reveal();
    $translation_service->_serviceId = 'string_translation';
    $entity_type->setStringTranslation($translation_service);

    $entity_type = unserialize(serialize($entity_type));
    $this->assertEquals('foo', $entity_type->id());
  }

I understand this is late feedback on a long-standing/RTBC issue which has limited time to get into 8.6, so please take this as observations/suggestions. If the preferred approach is #34 for now, please disregard.

Implementation of #37.

The test in #38 looks great to me.

Adding credit

Hopefully once we're on PHP7 (as is likely for 8.7.x) - we can add DependencySerializationTrait to StringTranslationTrait.

Before we commit this the issue title (I've had a go) and the issue summary need to be brought into line with the final patch. Imo we should get this into 8.6.x because it does improve and fix things for people.

Groomed the IS a bit.

As a major bug fix committed to 8.6.x.

Committed f4e81e0 to 8.7.x and a367e2c to 8.6.x. Thanks!

I used http://grep.xnddx.ru/search?text=extends+EntityType&filename= to ensure nothing in contrib actually extends EntityType

This introduced test failures on PHP 7.2:
- https://www.drupal.org/pift-ci-job/1036370
- https://www.drupal.org/pift-ci-job/1036469

1) Drupal\Tests\block\Functional\Update\BlockContextMappingUpdateFilledTest::testUpdateHookN
Exception: Notice: Drupal\Core\Entity\EntityType::__sleep(): The script tried to execute a method or access a property of an incomplete object. Please ensure that the class definition "Drupal\Core\StringTranslation\TranslatableString" of the object you are trying to operate on was loaded _before_ unserialize() gets called or provide an autoloader to load the class definition
Drupal\Core\Entity\EntityType->__sleep()() (Line: 34)


/var/www/html/core/lib/Drupal/Core/Test/HttpClientMiddleware/TestHttpClientMiddleware.php:51
/var/www/html/vendor/guzzlehttp/promises/src/Promise.php:203
/var/www/html/vendor/guzzlehttp/promises/src/Promise.php:156
/var/www/html/vendor/guzzlehttp/promises/src/TaskQueue.php:47
/var/www/html/vendor/guzzlehttp/promises/src/Promise.php:246
/var/www/html/vendor/guzzlehttp/promises/src/Promise.php:223
/var/www/html/vendor/guzzlehttp/promises/src/Promise.php:267
/var/www/html/vendor/guzzlehttp/promises/src/Promise.php:225
/var/www/html/vendor/guzzlehttp/promises/src/Promise.php:62
/var/www/html/vendor/guzzlehttp/guzzle/src/Client.php:131
/var/www/html/vendor/fabpot/goutte/Goutte/Client.php:155
/var/www/html/vendor/symfony/browser-kit/Client.php:312
/var/www/html/vendor/behat/mink-browserkit-driver/src/BrowserKitDriver.php:144
/var/www/html/vendor/behat/mink/src/Session.php:148
/var/www/html/core/tests/Drupal/Tests/UiHelperTrait.php:339
/var/www/html/core/tests/Drupal/Tests/UiHelperTrait.php:545
/var/www/html/core/tests/Drupal/FunctionalTests/Update/UpdatePathTestBase.php:334
/var/www/html/core/modules/block/tests/src/Functional/Update/BlockContextMappingUpdateTest.php:40

@tacituseu thanks! reverted this - very strange though that only 2 tests cause this.

Lolz... so core/modules/system/tests/fixtures/update/drupal-8.language-enabled.php has references to a class that was removed before 8.0.0 - nice. Changed Drupal\Core\StringTranslation\TranslatableString to Drupal\Core\StringTranslation\TranslatableMarkup.

It is interesting that only PHP 7.2 found this bug. I'm guessing this is due to performance improvements and when cache clearing occurs - but this is just a guess.

Nice sleuthing. Also did a quick grep, this looks like the only left over occurrence.

+++ b/core/tests/Drupal/Tests/Core/Entity/EntityTypeTest.php
@@ -476,4 +477,22 @@ protected function assertNoPublicProperties(EntityTypeInterface $entity_type) {
+   * Test the EntityType object it serializable.

Nitpick: the description of the test method could be formulated better:
Tests if the EntityType object is serializable.

Could be changed on commit.

Let's try this again. Committed and pushed 02330aaa20 to 8.7.x and a58773c021 to 8.6.x. Thanks!

diff --git a/core/tests/Drupal/Tests/Core/Entity/EntityTypeTest.php b/core/tests/Drupal/Tests/Core/Entity/EntityTypeTest.php
index ab02902db4..4027647abf 100644
--- a/core/tests/Drupal/Tests/Core/Entity/EntityTypeTest.php
+++ b/core/tests/Drupal/Tests/Core/Entity/EntityTypeTest.php
@@ -478,7 +478,7 @@ protected function assertNoPublicProperties(EntityTypeInterface $entity_type) {
   }
 
   /**
-   * Test the EntityType object it serializable.
+   * Tests that the EntityType object can be serialized.
    */
   public function testIsSerializable() {
     $entity_type = $this->setUpEntityType([]);

Fixed test comment.

@alexpott I updated the 'Version' to the earliest branch this commit is available in. This information relevant for projects that are built with patches from Drupal.org and that update to a new minor version (8.5.x -> 8.6.x), since it indicates the issue has been fixed in the project itself and the patch can be removed.

for D7:
#3093110: D7 - EntityType objects cannot be reliably serialized without DependencySerializationTrait

using php 7.3.8.1 I noticed an issue in one of my clients sites on a particular serialized data which is included in a test php file in the aforementioned issue. all you need is a command line to test. I think the issue is actually in the serialize() function. php 7.2.x and previous releases didn't throw an error , I tested the same client site on earlier versions of php prior to 7.3.8.1 and no issue what so ever (that I detected anyway).

Confused, why is this issue marked as "Fixed"?

patch 9 still applies cleanly to the head of 9.1.x and has tests!?

publish change record