Drupal\Core\Plugin\Context\Context needs DependencySerializationTrait

@Berdir
What did you need to do to make https://github.com/EdeMeijer/serialize-debugger work? Adjust the library? Adjust code in Drupal?

I get an error when trying the following code in a method called buildPaneForm() where serialize($this) failed:

\EdeMeijer\SerializeDebugger\Debugger::debugPlainText($this);die();

The error:

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 core/lib/Drupal/Core/Database/Connection.php).

call_user_func(Array) (Line: 38)
EdeMeijer\SerializeDebugger\Resolver\ObjectResolver->getProperties(Object) (Line: 23)
EdeMeijer\SerializeDebugger\Resolver\AbstractResolver->resolve(Object, Object) (Line: 42)
EdeMeijer\SerializeDebugger\Resolver\ChainingResolver->resolve(Object, Object) (Line: 30)
EdeMeijer\SerializeDebugger\Resolver\RecursiveResolver->resolve(Object, Object) (Line: 32)
EdeMeijer\SerializeDebugger\Resolver\RecursiveResolver->resolve(Object, Object) (Line: 32)
EdeMeijer\SerializeDebugger\Resolver\RecursiveResolver->resolve(Object, Object) (Line: 32)
EdeMeijer\SerializeDebugger\Resolver\RecursiveResolver->resolve(Object, Object) (Line: 32)
EdeMeijer\SerializeDebugger\Resolver\RecursiveResolver->resolve(Object, Object) (Line: 32)
EdeMeijer\SerializeDebugger\Resolver\RecursiveResolver->resolve(Object, Object) (Line: 32)
EdeMeijer\SerializeDebugger\Resolver\RecursiveResolver->resolve(Object, Object) (Line: 32)
EdeMeijer\SerializeDebugger\Resolver\RecursiveResolver->resolve(Object, Object) (Line: 32)
EdeMeijer\SerializeDebugger\Resolver\RecursiveResolver->resolve(Object, Object) (Line: 32)
EdeMeijer\SerializeDebugger\Resolver\RecursiveResolver->resolve(Object, Object) (Line: 32)
EdeMeijer\SerializeDebugger\Resolver\RecursiveResolver->resolve(Object, Object) (Line: 32)
EdeMeijer\SerializeDebugger\Resolver\RecursiveResolver->resolve(Object, Object) (Line: 55)
EdeMeijer\SerializeDebugger\Debugger->debug(Object) (Line: 70)
EdeMeijer\SerializeDebugger\Debugger::debugPlaintext(Object) (Line: 75)
Drupal\commerce_promotion\Plugin\Commerce\CheckoutPane\CouponRedemption->buildPaneForm(Array, Object, Array) (Line: 556)
Drupal\commerce_checkout\Plugin\Commerce\CheckoutFlow\CheckoutFlowWithPanesBase->buildForm(Array, Object, 'review') (Line: 46)
Drupal\ukkb_commerce\Plugin\Commerce\CheckoutFlow\Multistep->buildForm(Array, Object, 'review') (Line: 103)
Drupal\ukkbstudy\Plugin\Commerce\CheckoutFlow\UKKBMultistepEvent->buildForm(Array, Object, 'review')
call_user_func_array(Array, Array) (Line: 518)
Drupal\Core\Form\FormBuilder->retrieveForm('commerce_checkout_flow_ukkbstudy_multistep_event', Object) (Line: 275)
Drupal\Core\Form\FormBuilder->buildForm('commerce_checkout_flow_ukkbstudy_multistep_event', Object) (Line: 216)
Drupal\Core\Form\FormBuilder->getForm(Object, 'review') (Line: 94)
Drupal\commerce_checkout\Controller\CheckoutController->formPage(Object)
call_user_func_array(Array, Array) (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 582)
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 124)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext(Array, Array) (Line: 97)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 151)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 68)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 57)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 99)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1) (Line: 78)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 52)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 23)
Stack\StackedHttpKernel->handle(Object, 1, 1) (Line: 666)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)

@Berdir
Thanks for your quick reply. I was expecting that the library would stop on an exception and then print where things went wrong.

In my case \Drupal\Core\Entity\EntityTypeManager apparently is the culprit. When inspecting each \EdeMeijer\SerializeDebugger\Node in the trace, I got a chain of the following objects:

Drupal\Core\Database\Driver\mysql\Connection
Drupal\locale\StringDatabaseStorage
Drupal\locale\LocaleTranslation
array(0)
Drupal\Core\StringTranslation\TranslationManager
Drupal\Core\Entity\EntityTypeManager
Drupal\ukkbaccount\Account\Profile
Drupal\ukkb_commerce\Plugin\Commerce\CheckoutPane\ContactInformation
Drupal\commerce_promotion\Plugin\Commerce\CheckoutPane\CouponRedemption

Ah, thus services should generally not be serialized at all.

\Drupal\ukkbaccount\Account\Profile is a custom object with several account related methods. Its constructor asks for the entity type manager and an account. Adding a DependencySerializationTrait there solves the issue in my case! An ajaxified form element now gets properly updated.

Would it be a good idea for a better developer experience to throw exceptions in __sleep() in all service classes as well? This way more developers will be informed that you should avoid serializing these.

This will need tests...

This blocks #2976148: Layout-based entity rendering should delegate to the correct section storage instead of hardcoding to either defaults or overrides

Here, have a patch with a test!

1. +++ b/core/lib/Drupal/Core/Plugin/Context/Context.php
   index aac6efc0e3..d32d6d5653 100644
   --- a/core/tests/Drupal/Tests/Core/Plugin/Context/ContextTypedDataTest.php
   
   --- a/core/tests/Drupal/Tests/Core/Plugin/Context/ContextTypedDataTest.php
   +++ b/core/tests/Drupal/Tests/Core/Plugin/Context/ContextTypedDataTest.php
   
   +++ b/core/tests/Drupal/Tests/Core/Plugin/Context/ContextTypedDataTest.php
   @@ -16,7 +15,7 @@
   +class ContextTypedDataTest extends KernelTestBase {
   

   If this moves from a unit to kernel test, it should move to core/tests/Drupal/KernelTests/Core/Plugin as well (or just make a new class)

2. +++ b/core/tests/Drupal/Tests/Core/Plugin/Context/ContextTypedDataTest.php
   @@ -25,6 +24,22 @@ class ContextTypedDataTest extends UnitTestCase {
   +    $this->typedData = new StringData($data_definition);
   

   Weird using an object property for this instead of a local variable.

3. +++ b/core/tests/Drupal/Tests/Core/Plugin/Context/ContextTypedDataTest.php
   @@ -25,6 +24,22 @@ class ContextTypedDataTest extends UnitTestCase {
   +    // getContextValue() will cause the context to reference the typed data
   +    // manager service. Without DependencySerializationTrait, this will cause
   +    // the context to fail on serialization.
   

   This comment is a little odd because it references how code used to be instead of how things are.

4. +++ b/core/tests/Drupal/Tests/Core/Plugin/Context/ContextTypedDataTest.php
   @@ -25,6 +24,22 @@ class ContextTypedDataTest extends UnitTestCase {
   +    $context->getContextValue();
   +    serialize($context);
   

   I think we need something asserted here.

For the record, we could solve all future occurrences of this problem if we would never make service getters into implicit setters.

function getTypedDataManager() {
  if (!$this->typedDataManager) {
    $this->typedDataManager = \Drupal::typedDataManager();
  }
  return $this->typedDataManager;
}

This is an anti-pattern: it's not a getter, it's a setter. And we wonder why we run into sudden serialization failures. We should reject this anti-pattern forever (in fact, we should deprecate it in Drupal 8, and remove it in Drupal 9). Of course, it'd still be possible to cause a serialization failure by explicitly setting the service (setTypedDataManager()) and serializing the consuming object, but at least then it would be traceable to an explicit method call somewhere, and could be addressed by module authors rather than the onus being on core.

In any case, the method says get, but it actually sets. We need to not do that. Not in this issue, necessarily, but we should not allow any more instances of this anti-pattern to land in core. Looks like I found the hill I'm willing to die on!

All feedback from #11 addressed here. I moved the test to the KernelTests namespace and rewrote the mock object to use Prophecy.

Looks great, thanks!

Whoops! I had the test in the wrong namespace. Kicking directly back to RTBC on the assumption that it will go green this time.

Adding issue credits

Committed b1ad4a8 and pushed to 8.7.x. Thanks!

C/p as c0c662c427 and pushed to 8.6.x

Thanks

Thanks, @larowlan!

For anyone who's interested, I opened an issue pleading with the framework managers to deprecate the "implicit setter" anti-pattern which caused this bug: #3011620: Deprecate the "implicit setter" anti-pattern

This introduced test failure on PHP 5.5:
https://www.drupal.org/pift-ci-job/1113676
https://www.drupal.org/pift-ci-job/1113681

1) Drupal\KernelTests\Core\Plugin\ContextTypedDataTest::testGetContextValue
PHPUnit_Framework_Exception: PHP Fatal error:  Call to undefined method Prophecy\Prophecy\MethodProphecy::shouldBeCalledOnce() in /var/www/html/core/tests/Drupal/KernelTests/Core/Plugin/ContextTypedDataTest.php on line 53
Fatal error: Call to undefined method Prophecy\Prophecy\MethodProphecy::shouldBeCalledOnce() in /var/www/html/core/tests/Drupal/KernelTests/Core/Plugin/ContextTypedDataTest.php on line 53

That's shouldBeCalledTimes(1) in older PHPSpec.

But it's unnecessary here anyway.

Committed and pushed bb9fb66dd7 to 8.7.x and d935fc4871 to 8.6.x. Thanks!

Ran tests locally on PHP5.5,5.6 and 7 and they pass.