Serializing the container is a very very bad idea, let's prevent it?

Wow.

Views with exposed filters seem to trigger this
admin/content
admin/people

@Berdir found that this helps.

Found that this also helps for CKEditorAdminTest.
I don't see why editor_form_filter_format_form_alter() has a need to serialize the Editor(Plugin)Manager.

Note that without this fix, a very weird $manager comes out of unserialize():
$manager->factory->discovery == 'breakpoint' (?! - it should be the EditorManager object itself)

Which makes me think that our current DependencySerialization code has flaws - maybe on cases of circular references like $service->foo->bar === $service.
[edit: couldn't isolate this on a simpler case though]

Also, the var_dump() debug code in PluginBase is handy but breaks AJAX :-)
Left it commented so that testbot gives us accurate results [edit: and cancelled test for #6]

(CKEditorAdminTest still fails with the changes from #7, but it fatals earlier without them)

Let me try this. I've got some documentation/string fixes at least.

I updated some code comments and the exception message to be a bit more professional and descriptive. I also prevented container serialization for two services, but the test still fails.

Serializing *any service* seems like a really bad idea. When unserializing, you will get a *second copy* of the service, potentially with a different configuration (and/or internal state).

On the other hand, serializing a reference to a service (aka. the service name) is fine and probably useful.

This probably requires some thinking, because the hack that is DependencySerialization is going to possibly miss a lot of references to services.

@Damien:

This probably requires some thinking, because the hack that is DependencySerialization is going to possibly miss a lot of references to services.

DependencySerialization cannot miss any service since the container adds the _serviceId property to all of them?

@yched: The crap in editor_form_filter_format_form_alter() has been fixed in another issue: #2182077: Editor manager in $form_state causes test failures on PHP 5.4

@Xano: The goal is not to add __sleep() methods willy-nilly all over the place, but fix things as upstream as possible.

That said, this interdiff is agains #5 and it should have only two failing tests, CKEditorAdminTest and ConfigTranslationUiTest.

@amateescu: yes, that will work for services that are top-level properties, not to anything deeper then that.

We don't want to serialize any services, but we won't be able to not serialize form objects, entities, plugins, entity controllers and so on that do reference services.

@Berdir: in this list, it is arguable that we really should prevent serializing plugins and entity storage controllers. Those two can have an internal state (entity storage controllers have, at least, a cache), and allowing them to be (un)serialized is a recipe for hard-to-debug bugs.

@Damien, that's why the patch adds DependencySerialization to EntityControllerBase and (copies it, until we have a trait) to PluginBase.

that will work for services that are top-level properties, not to anything deeper then that

nope, DependencySerialization does catch nested services, since serialize() runs __sleep() & serialize() recursively on each referenced object.

@yched, I think @Damien means that DependencySerialization doesn't help in cases where a property can hold an object which is not itself a service, but holds a reference to a service. This is exactly the case with Drupal/Core/Config/Config, which I had to fix in #17.

Also, this thing about "we should not serialize services to begin with" has been discussed at length before. The conclusion was that Form API makes this unavoidable without major rework - and D8's Form API even more, since ;
- FormControllerBase does $form_state['form_controller'] = $this
- $form['#some_callback'] = array($this, 'someMethod') is supported and very tempting

That led to DependencySerialization as a sorry safeguard.

The issue with DependencySerialization, as either a base class or as a trait, is that :

- it's left to each class to figure out "oh, turns out some weird code somewhere is serializing me because of a form or whatever, so I'd better use DependencySerialization so it doesn't mess up my service dependencies, + some custom __sleep() code to deal with my properties that happen to be static caches or similar".

- "some weird code somewhere is serializing me" is extremely easy to simply go unnoticed in the first place.

[crosspost with #22 - in short, yes, DependencySerialization only works on objects that extend DependencySerialization...]

Apparently it's hard to get this point across. DependencySerialization only work to, erm, serialize the direct dependencies of some object. It doesn't handle the object itself.

As a consequence:

• Anything deeper (for example a service referenced in a array property of the object) is going to be serialized as-is.
• The object itself and all the rest of its internal state is going to be serialized.

You don't actually want to allow serializing most of the services, you want to serialize *references* to those services (which is essentially what DependencySerialization does, but for a very narrow use case).

Allowing services to be serialized is a recipe for failure, so we should probably at the minimum add a __sleep to some of them and see what breaks.

Not hard to get across, #26 and #23 are saying the same thing :-p

#29 raises valid concerns. But not sure of how we address them.

Yes, a flat exception / fail leaves you with no clues as to what causes the bug, or even against which component / module to report it. Even a callstack is of little help, it's not the code that triggers serialization that is at fault.
We could look for a couple typical serialization callstacks in core and display the form_id, or the key of the k/v record... - kind of ugly though :-/ We cannot even report the path to the container in the overall serialized object.
So, yeah, what we'd really need is print @Berdir's phone number in the exception message...

Having this silently work IRL but fail in tests would be a bit confusing when trying to debug a failed test ? ("cannot reproduce the failure outside of the test"). Do we already have similar behavior on other stuff ?

Not really a review.

1. +++ b/core/lib/Drupal/Core/DependencyInjection/ContainerBuilder.php
   @@ -89,4 +89,17 @@ protected function callMethod($service, $call) {
    
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function get($id, $invalidBehavior = self::EXCEPTION_ON_INVALID_REFERENCE) {
   +    $service = parent::get($id, $invalidBehavior);
   +    // Some services are called but do not exist, so the parent returns nothing.
   +    if (is_object($service)) {
   +      $service->_serviceId = $id;
   +    }
   +
   +    return $service;
   +  }
   +
   

   Where is the place we use the container builder instead of a dumped container instance?

2. +++ b/core/lib/Drupal/Core/DependencyInjection/DependencySerialization.php
   @@ -33,6 +34,11 @@ public function __sleep() {
            $this->_serviceIds[$key] = $value->_serviceId;
            unset($vars[$key]);
          }
   +      // Special case the container, which might not have a service ID.
   +      elseif ($value instanceof ContainerInterface) {
   +        $this->_serviceIds[$key] = 'service_container';
   +        unset($vars[$key]);
   +      }
        }
    
   

   It would be great to expand the existing unit test.

Note that I had opened #2095203: DependencySerialization should work with ContainerBuilder as well, not just Container already at some point when config_translation mappers were still storing the entity manager. We might close that as duplicate now.

Also, that issue contains test coverage, which we could bring over here.

+++ b/core/lib/Drupal/Core/DependencyInjection/Container.php
@@ -27,4 +27,12 @@ public function get($id, $invalidBehavior = self::EXCEPTION_ON_INVALID_REFERENCE
+  function __sleep() {

+++ b/core/lib/Drupal/Core/DependencyInjection/ContainerBuilder.php
@@ -89,4 +102,12 @@ protected function callMethod($service, $call) {
+  function __sleep() {

Let's just make them public as well.

Thank you

Committed 1cce899 and pushed to 8.x. Thanks!

I'm sorry to be posting on a closed issue, but I'm wondering if someone can point me toward any follow-up on the points in number #26? Specifically this point:

Anything deeper (for example a service referenced in a array property of the object) is going to be serialized as-is.

Am I to understand that this is still the case? Does this mean that it is not good practice to include any properties that may contain their own injected services within an object that might get serialized? I'm working with a views style plugin at the moment (extends Drupal\views\Plugin\views\style\StylePluginBase) which is something that appears to be serialized as part of various views UI form operations. If I add a helper object that contains its own injected services as a property of this plugin all kinds of problems arise. These problems are clearly related to this issue as their calling card is "The container was serialized" errors in the logs.

Perhaps there is a follow-up that deals with this situation that I could explore? The serialization of objects that contain properties, that further contain services, must be an allowable practice?

Thanks, yes I see now that views style plugins contain DependencySerializationTrait. Unless I'm mistaken though, that does not seem to recursively look for services within the plugin's properties, so services that are nested inside another property on the plugin could still cause problems upon serialization.

Are you saying that this "nested" case has already been accounted for somewhere? If so perhaps I've found a regression. Or, are you saying that core is already doing all that it should be doing and that this idea of "nested" properties (again, services inside of other properties that are themselves not services) must be avoided by anyone implementing plugins?

The point is your nested object itself needs to implement the DependencySerializationTrait - that is how to fix this.

As soon as you do:

$myobject = $otherobject->service

Or

otherobject::doSomething() {
$this->someObj = new MyObject($this->someService);
}

then MyObject needs to implement the DependencySerializationTrait itself for this to work.

That is how it works.

I would argue in that case you need to implement __wakeup and __sleep yourself.

Deep array search is usually very costly, so don't think we want to go that route.

If I recall correctly, what I had was along these lines:

$some_core_service = $my_views_plugin->my_helper_object->some_core_service

Such that my_helper_object contained some helper methods that were also used by another display formatter that my module implemented. These methods needed some_core_service(s) for config management and URL generation, so I simply injected them into my_helper_object instead of calling Drupal::service internally. Initially I reasoned that I should not make my_helper_object its own service because it was only applicable within the internal architecture of my module. Since then however, I've simply refactored it into a service. As best I can tell, that would be considered best practice for this kind of thing.

I see now that I could have added DependencySerializationTrait, as suggested in #47. To be honest, back then I just didn't understand how this serialization could have related to views UI forms (and how on earth the serialization of the actual container could have related to the processing of those forms).

I just didn't understand how this serialization could have related to views UI forms (and how on earth the serialization of the actual container could have related to the processing of those forms).

Yep, that's not new, but it's exactly the issue with OO forms in D8 : it makes it scaringly easy to end up serializing objects that were never written with serialization in mind, and generate huge serialization strings if they have deep object reference chains. And this can stay unnnoticed for months :-/

Yep, that's not new, but it's exactly the issue with OO forms in D8 : it makes it scaringly easy to end up serializing objects that were never written with serialization in mind, and generate huge serialization strings if they have deep object reference chains. And this can stay unnnoticed for months :-/

@alexpott doesn't agree with you in #2430813-24: Selecting the 'views' selection handler on an entity_reference field causes a fatal error and the comments below that. I'm tired of arguing, I hope you'll have better luck.