Injected dependencies and serialization hell

Shortly discussed that with crell, who said that's the best we can do. He suggested going with the Serializable interface though.

[edit: copied in the OP]

Bon Dieu...

Drupal serializes a crapload of stuff, that's true. Is there any way we can reduce what gets serialized? And/or only serialize pure data objects, at least in some cases, so that we side-step this problem? (Pure data objects = no dependencies, like the Request object.)

I'll ping some Symfonians and see if they have insights.

the clean way is to serialize only data-objects. This would solve most serialization issue (except complex references in these data-objects if the same object appears in several places though).
As soon as services appear in the object graph, a proper unserialization needs to get the current instance in the container, which cannot be done in PHP (well, it could be hacked in drupal thanks to the globally accessible container, as suggested by @yched, but it is far from being clean).

Note that Symfony itself faces this issue. See https://github.com/symfony/AsseticBundle/issues/168 for instance, which cannot be solved as long as the serializable resource need to be able to use Twig. This is why https://github.com/symfony/symfony/pull/7230 exists to try finding an issue.

"Only serialize pure data objects":
I'm afraid this ship has sailed.
- The whole FormInterface construct rely on serializing the FormInterface object, which is typically a (request) controller,
- Entity forms more specifically serialize the EntityFormController (controller in the 'entity' sense, nor "request"),
(+ the entity itself, and the EntityFormDisplay (config entity) - but those two currently can't receive injected dependencies)

"Limiting what gets serialized":
Not sure that's sustainable - at least it hasn't been so far :-).
The problem is also that supporting '#callback' => array($some_object, 'someMethod') (possibly set by 3rd party code in a form_alter()) arbitrarily broadens the scope of what can end up being serialized.
The existence of a DrupalSerializableInterface, based on the serialize()/unserialize() behaviors described in #2, might help narrowing the set of objects we actually support here...

And that's only for forms. Not sure about serialization / caching of render arrays, or caching of other stuff, or things placed in state / k/v...

Thanks for writing that down.

Another level of complexity is created if we talk about context of plugins instead of simple dependency injection.

Just an idea: If we have a custom serialization interface form_get_cache could look like:

  if ($form = Drupal::keyValueExpirable('form')->get($form_build_id)) {
    global $user;
    if ((isset($form['#cache_token']) && drupal_valid_token($form['#cache_token'])) || (!isset($form['#cache_token']) && !$user->uid)) {
      if ($stored_form_state = Drupal::keyValueExpirable('form_state')->get($form_build_id)) {
        foreach ($stored_form_state['build_info']['objects'] as $object) {
          if ($object implements DrupalSerializationInterface) {
            $object->recreate(\Drupal::getContainer());
          }
        }

"Only serialize pure data objects":

Also - our primary data objects (entities and fields) contain logic and should not stay pure, we want to inject dependencies there as well. So I agree that re-pulling services from the global DIC is the best we can do.

So I should probably have created a separate issue for my rant in #2, but it has now effectively taken the issue over. Sorry about that @tim.

I went ahead and moved #2 to the OP, and adjusted the issue title accordingly.
I created #2020385: Implement Serializable for entity controllers to keep track of the original need.

I also think this is at least major, possibly critical.

Issue hijack...

@amateescu pointed at another nasty occurrence - PHP 5.4 only : #2025451: Drupal\entity_reference\Tests\EntityReferenceAdminTest fails on PHP 5.4.9 and libxml 2.9.0

Because I promised @yched and to get it off my mind...

Note that this does *not* fix #2025451: Drupal\entity_reference\Tests\EntityReferenceAdminTest fails on PHP 5.4.9 and libxml 2.9.0 and that it should also be used by #2059245: Add a FormBase class containing useful methods.

Also, I deviated a bit from the proposed resolution in the OP by creating a base class. I think this is much more useful than writing it again and again in each "DIC serialization friendly" class.

Umm, no, PluginManagerBase is in Component.

This can't work because the compiled container uses a totally different class.

Here is an alternative approach.

Additional I also think that we should just store all the services in one key, so unserialization is a bit faster. Just iterate over $this->_services and get it from the container.

Additional I also think that we should just store all the services in one key, so unserialization is a bit faster.

What exactly will be faster, is there a getMultiple() that I'm not seeing?

You don't have to iterate over each entry of the class, but just over the actual services.

This time with an actual working implementation.

And now the serialized string has a "magic" __services property and the code did not get prettier IMO. Maybe some profiling would help to see if iterating over each member of the class is actually a problem first?

As discussed in IRC with @dawehner, I agree with using a separate property to store the serialized IDs, mostly for two additional reasons:

- It just feels wrong to replace properties that are documented as \Drupal\SomeClass with a string, even if it's never visible during runtime. That PHP allows it doesn't mean we should mis-use it IMHO.

- It could lead to very crazy bugs: $this->useBackend = 'database'. And on unserialize, that's going to be replaced by the database connection :)

@@ -0,0 +1,60 @@
+    $services = array();
...
+        $services[$key] = $value->__serviceId;
...
+    if ($services) {
+      $object->__services = $services;
+    }
...
+    $services = isset($data['__services']) ? $data['__services'] : array();
+    unset($this->__services);

Either document __services as a property or don't bother with conditionally setting/unsetting it. Not both :)

That would simplify the code quite a bit.

Good point, here is a simplification of that point as well as fixing the bugs which appeared.

Thanks so much for attacking this folks, I intended to, but I'm swamped with the Field API queue...

When we discussed it with @Crell in NYCCamp, he was very reluctant on having \Drupal::getContainer() in tons of classes (all those who extend DependencySerialization).

The alternative we can up with was (functions / method / interface names are mere proposals):
- Introduce custom drupal_serialize() / drupal_unserialize() functions
Those functions are used in place of PHP's serialize() / unserialize() by code that wants to serialize objects in a "DI safe" manner (e.g form.inc)
They take care of recursion in arrays & objects
- They trigger custom DrupalSerializableInterafce methods in objects rather that PHP's Serializable::serialize() / unserialize()
- drupal_unserialize() does $container = \Drupal::getContainer() and injects as a param in DrupalSerializableInterafce::unserialize()
The objects themselves are then not container aware, drupal_unserialize() says "repopulate yourself with this container"

Wow this sound quite a lot of more complexity to just not use Drupal::getContainer() in a single base class.

Let's see whether the maintainers will accept that API change.

Well, it's a base class so the method will actually live in lots of objects...
I personally tend to send see having classes pull the container *in the specific context of unserialization* as not really problematic either, but Crell was strongly against.

I personally tend to send see having classes pull the container *in the specific context of unserialization* as not really problematic either, but Crell was strongly against.

We could get this patch in and then iterate on top of it. We need all of the logic in the patch anyway ... Discussions for the drupal_serialize API change will come up etc.

It seems okay to use Drupal::methods for static methods (we use it all over the place in entities), so it should be kind of okay for this special methods as well.

+++ b/core/lib/Drupal/Core/DependencyInjection/DependencySerialization.phpundefined
@@ -0,0 +1,58 @@
+    foreach ($this->__services as $key => $service_id) {

We should test the existence of $this->__services, it won't be set if serialize encountered no services.
[edit: ignore me, I'm stupid]

+++ b/core/lib/Drupal/Core/DependencyInjection/DependencySerialization.phpundefined
@@ -0,0 +1,58 @@
+ * Provides a dependency injection serialization friendly class.

Nitpick: "dependency injection friendly methods for serialization" ?

+++ b/core/modules/field_ui/lib/Drupal/field_ui/Form/FieldInstanceEditForm.phpundefined
@@ -187,6 +188,8 @@ public function buildForm(array $form, array &$form_state, FieldInstanceInterfac
+    $this->instance = $form_state['instance'];

I guess this is added only as proof that the patch works, but should not be part of the commit ?

Nitpick again: for consistency with the __serviceId property added in services, the __services property used to store service ids in serialized objects could be named __serviceIds ?

Nitpick again: for consistency with the __serviceId property added in services, the __services property used to store service ids in serialized objects could be named __serviceIds ?

I like that

Let's add some unit tests to ensure it works as expected.

#28: drupal-2004282-28.patch queued for re-testing.

Forgot a bit of the review..

Forgot the interdiff.

We should probably see if this lets us remove the serialize() / unserialize() stuff that had to be added to DrupalKernel in #1909418: Allow Entity Controllers to have their dependencies injected ?

We should probably see if this lets us remove the serialize() / unserialize() stuff that had to be added to DrupalKernel in #1909418: Allow Entity Controllers to have their dependencies injected ?

This does not work, because the drupal kernel is initialized outside of the container world, see

function drupal_handle_request($test_only = FALSE) {
  // Initialize the environment, load settings.php, and activate a PSR-0 class
  // autoloader with required namespaces registered.
  drupal_bootstrap(DRUPAL_BOOTSTRAP_CONFIGURATION);

  // Exit if we should be in a test environment but aren't.
  if ($test_only && !drupal_valid_test_ua()) {
    header($_SERVER['SERVER_PROTOCOL'] . ' 403 Forbidden');
    exit;
  }

  $kernel = new DrupalKernel('prod', drupal_classloader(), !$test_only);

  // @todo Remove this once everything in the bootstrap has been
  //   converted to services in the DIC.
  $kernel->boot();

Yes, but i thought maybe the fact that we stopped serializing services means in turn we stop hitting a case where we find ourselves serializing the kernel that happens to be referenced in one of those services ?

Point being, I'd tend to consider that being in a situation where you find yourself serializing the kernel is the sign that you're really doing something wrong, we should let that fail instead of making it silently work.

Let's see what the bot things of doing it.

DrupalKernelInterface should stop extending Serializable then ?

Good catch. It would have been nice to throw something if someone really tries to.

One open question is: what kind of classes will use that baseclass. We could potentially go with ControllerBase, the potential FormBase as well as PluginBase ...

It would have been nice to throw something if someone really tries to

True - I'd also be in favor of throwing an exception if someone tries to serialize a plugin manager, that's an insane thing to do...

what kind of classes will use that baseclass. We could potentially go with ControllerBase, the potential FormBase as well as PluginBase ...

Well, ControllerBase holds a ref to the container, so safe-serializing it would even be a little more involved - I'm not sure we really have cases were a controller would end up serialized though ?
Forms most definitely, they are the #1 thing on the top of the serialization chain.
PluginBase would be a good thing too, EntityControllers... - basically anything that can have a create() factory method ?

Right, controllers should not life outside of the http kernel. PluginBase is a bit complicated as it is part of the components so you would have to create a new PluginBase which extends From SerializationBase
and copy all the code.

PluginBase is a bit complicated as it is part of the components so you would have to create a new PluginBase which extends From SerializationBase

True... Still, better to do that in a Core/Plugin/PluginBase (extending Component/Plugin/PluginBase) rather than provide nothing and have each plugin type [edit: each plugin implementation class, actually] having the same dilemma (copy methods from PluginBase or from DependencySerialization) ?

What about making a new follow up for talking about the case of plugins? This potentially will change all of the existing plugin base classes etc.

Yes, sounds more reasonable...

Opened a follow up #2070309: Figure out how to solve serialization hell for plugins

In other news, we've been facing worrying behavior differences with Serialize interface between PHP 5.3 & 5.4 :-(
[edit: doh, forgot to link to the issue... It's the one @claudiu.cristea links in the next comment]

Related: #2071969: Serialize interface not reliable.

Just a normal rerole.

Per @chx, it is not allowed to use __, that is reserved for PHP. Use _serviceId instead.

It's more of a convention than anything else but this convention is one we want to keep.

A few more nitpicks:

+ * Provides a dependency injection friendly methods for serialization.

a is not necessary

$object->{$key}

when there is no ambiguity on the meaning of the expression{} is not needed. $foo->$bar['baz'] would mean that $bar is an array and $bar['baz'] is calculated first but $object->$key can't be misinterpreted.

Finally, why does serialize run a clone and two get_object_vars...? Why it doesn't run a single get_object_vars and manipulate the array to its heart content?

Good points chx. Here are some fixes.

Formatting.

I didn't see why #54 fail, but here's a code clean try. The first failed test in #54 passes locally with this new patch.

+++ b/core/lib/Drupal/Core/DependencyInjection/DependencySerialization.php
@@ -23,16 +23,14 @@
-      }
-      elseif ($key != '_serviceIDs') {
-        $var[$key] = $value;
+        unset($var[$key]);

This is indeed quite simpler!

+++ b/core/lib/Drupal/Core/DependencyInjection/DependencySerialization.phpundefined
@@ -0,0 +1,55 @@
+   */
+  protected $_serviceIDs = array();

Nit. Normalize the spelling of _serviceId vs. _serviceIDs.

+++ b/core/lib/Drupal/Core/DependencyInjection/DependencySerialization.phpundefined
@@ -0,0 +1,55 @@
+  public function serialize() {
+    $var = get_object_vars($this);
+    foreach ($var as $key => $value) {
+      if (is_object($value) && isset($value->_serviceId)) {
+        // If a class member was instantiated by the dependency injection
+        // container, only store its ID so it can be used to get a fresh object
+        // on unserialization.
+        $var['_serviceIDs'][$key] = $value->_serviceId;
+        unset($var[$key]);
+      }
+    }
+
+    return serialize($var);

I think either $var['_serviceIDs'] needs to be initialized to an empty array here, or $this->_serviceIDs needs to be set to an empty array at the end of unserialize. I prefer the former so that people cannot mess with $this->_serviceIDs.

It's possible that an object will have different stored services during two different serialize() calls.

Rewrote it using __sleep and __wakeup, though the problem is that __sleep just works with member variables, so you can't just put arbitrary values on objects anymore which I think drupal does here and there.

__sleep() looks wrong, its return value should be the array of property names that will be included in the serialization:
array('foo', 'bar', '_serviceIDs');
(yes, __sleep() is a bit weird...)

Oh.

I think the latest patch should look this:)
Edit:

+    $keys = array_keys($vars);
+    foreach ($vars as $key => $value) {
+      if (is_object($value) && isset($value->_serviceId)) {
+        // If a class member was instantiated by the dependency injection
+        // container, only store its ID so it can be used to get a fresh object
+        // on unserialization.
+        $this->_serviceIDs[$key] = $value->_serviceId;
+        unset($vars[$key]);
+      }
+    }
+
+    return $keys;

$keys should unset the services key here.

Anyway, I don't think __sleep / __wakeup is a good idea here imho.

a) Patch introduced at #2071969: Serialize interface not reliable is to fix the serialization issue when it comes to object property that references to another obejct implementing method serialize() too. But in our case here, we are trying to avoid serialize this kind of properties( dependency services), so we use unset() to eliminate these properties from serialize process. The only benifit I could see is to avoid other properties, except the services, have potential problems with serializable interface.

b) Maybe some properties, except the eliminated services, have been set private in declaration, which will lead to following problem:

It is not possible for __sleep() to return names of private properties in parent classes. Doing this will result in an E_NOTICE level error. Instead you may use the Serializable interface.

Considering a) and b), I'd like to preserve serializable interface here and dig it at #2074253: Fatal error when using Serializable interface on PHP 5.4:)

Well, using private properties is considered as practice anyway, so I don't see a big issue with that.

@smiletrl: your reasoning only holds when the object using DependencySerialization is the top-level object being serialized, not when it's being serialized as part of recursive serialization of a larger array ($form, $form_state...).
Also, this patch here only unset()s referenced objects *that are services*, not other objects that might be referenced in the serialized object. Those will still be serialized as usual.

So no, I do think Serialize interface is unsafe here.

@yched, yeah, right. I missed a lot here:)

So @yched, do you think this issue is ready to fly?

As this fixes a lot of test failures of #2068471: Normalize Controller/View-listener behavior with a Page object this is also critical.

Mostly nitpicks:

1. +++ b/core/lib/Drupal/Core/DependencyInjection/Container.php
   @@ -0,0 +1,30 @@
   +use Symfony\Component\DependencyInjection\Container as ContainerBase;
   

   Uber nitpick, feel free to disagree & ignore:
   FooBase is what we use for base classes, so feels a bit weird for an alias to the symfony Container.
   Maybe SymfonyContainer ?
   (same in ContainerBuilder.php)

2. +++ b/core/lib/Drupal/Core/DependencyInjection/DependencySerialization.php
   @@ -0,0 +1,50 @@
   +  public function __sleep() {
   

   As @twistor mentioned in #58, I think we'd need to reset $this->_serviceIDs to an empty array at the beginning of the method (case of successive serialization of the same object in possibly different states). Emptying it after it has done its job in __wakeup() might be a good idea too (needless clutter)

   (Also agreed with his remark that _serviceIDs should be _serviceIds for consistency with $service::_serviceId)

3. +++ b/core/tests/Drupal/Tests/Core/DependencyInjection/DependencySerializationTest.php
   @@ -0,0 +1,98 @@
   +  /**
   +   * The tested dependency serialization instance.
   +   *
   +   * @var \Drupal\Core\DependencyInjection\DependencySerialization
   +   */
   +  protected $dependencySerialization;
   

   $this->dependencySerialization is created and used all within the testSerialization() test method, so maybe it doesn't need to be a property in $this and we can save documenting it ?

4. +++ b/core/tests/Drupal/Tests/Core/DependencyInjection/DependencySerializationTest.php
   @@ -0,0 +1,98 @@
   +    // Create a pseudo service and
   

   unfinished sentence ?

5. +++ b/core/tests/Drupal/Tests/Core/DependencyInjection/DependencySerializationTest.php
   @@ -0,0 +1,98 @@
   +    $service = new \stdClass();
   +    $service->key = 'value';
   +    $service->_serviceId = 'test_service';
   +
   +    $this->dependencySerialization = new TestClass($service, 'example');
   

   To better demonstrate the behavior, should we:
   - not explicitly set $service->_serviceID
   - create the test object with new TestClass($container->get('test_service'), 'example') ?
   - test that $service->_serviceId == 'test_service' after that ?

6. +++ b/core/tests/Drupal/Tests/Core/DependencyInjection/DependencySerializationTest.php
   @@ -0,0 +1,98 @@
   +    $this->assertEquals($service, $object->service);
   

   Shouldn't this be assertIdentical() ? (what we want to test is identity, not just equality)

This is trying to address concerns at #69.

Emptying it after it has done its job in __wakeup() might be a good idea too (needless clutter)

It might be ok to preserve the _serviceIds property for object, after it's been serialized: a) code could use _serviceIds to target all injected services quickly after serialization; b) Currently, _serviceIds only works with serialization, and it would be initiated as an empty array before serialization (in __sleep). So, I guess it's not necessay to empty it again.

Rewrite TestClass to make use of the popular 'create' method to inject dependencies.

Haven't tested it locally, see if bot likes it:)

Thanks @smiletrl.

- No need for the create() factory method in the test, this is completely unrelated to the code being tested, and it makes the test harder to read for no reason. Creating an object with new Class($dependency) is perfectly legit, the "create() factory method" is nothing more than a pattern that core uses in a couple places where it makes sense, but it doesn't here.

- I still vote for emptying _serviceIds at the end of wakeup(). This patch is only about ensuring correct unserialization, not about providing "oh, when an object has been unserialized, you can find this _serviceIds property in it and do interesting stuff with it" - that's not a behavior we want to support and maintain.
So -1 to introducing a "state" about "the object has been through unserialization". Let's cleanup after ourselves.

- And still +1 for emptying _serviceIds at the beginning of __sleep() too. An object continues its life throughout the request after serialized has been called on it. A second serialization of the object later in the request should not build on the _serviceIds left by the first serialization, the object might have changed state (i.e $this->some_service that was set the 1st time may now be NULL). Might not cause actual bugs with the current code, but it's much safer to make sure of it. No reason to persist a state of _serviceIds that was only relevant at the specific time the object was serialized.

• Removed the create method bit
• Removed _serviceIds at the end of wakeup()
• Added a test to test just the container specific behavior.

Thanks for clearing 'create':)

Some other nitpicks:

1).

+    // The original object got _serviceIDs added so let's remove it to check
+    // equality
+    unset($dependencySerialization->_serviceIds);

_serviceIDs VS _serviceIds.

2).

+    $service = new \stdClass();
+    $service->key = 'value';
+    $service->_serviceId = 'test_service';
+    $container = $this->getMock('Drupal\Core\DependencyInjection\Container');
+    $container->expects($this->exactly(1))
+      ->method('get')
+      ->with('test_service')
+      ->will($this->returnValue($service));
+    \Drupal::setContainer($container);
+
+    $dependencySerialization = new TestClass($service, 'example');

As @yched mentioned at #69, point 5), I agree here should use $container->get('test_service') as paramter injected into TestClass. Otherwise, the mocked $container is useless here.

3). As mentioned above,
+ $service->_serviceId = 'test_service';
should probably be deleted and it should be tested after service gets injected. This property _serviceId will be assigned value at $container->get(), which is exactly what we create a new container for:

+class Container extends SymfonyContainer {
+
+  /**
+   * {@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;
+  }

I guess code at #70 has done it properly, i.e. deleting it and testing it,?

4).

+  public function __wakeup() {
+    $container = \Drupal::getContainer();
+    foreach ($this->_serviceIds as $key => $service_id) {
+      $this->$key = $container->get($service_id);
+    }
+    unset($this->_serviceIds);
+  }

Perhaps emptying, i.e., $this->_serviceIds = array(); is a better idea over unset? "_serviceIds" is an intrinsic property of abstract class DependencySerialization other than some arbitrary property assigned during its life cycle. And the default value has been assigned an empty array. I guess it's better to keep consistency here, which is the same case at the beginning of __sleep().

5).

+    // The original object got _serviceIDs added so let's remove it to check
+    // equality
+    unset($dependencySerialization->_serviceIds);

If we use $this->_serviceIds = array(); at __wakeup(), then this unset code could be deleted.

+    // Ensure that _serviceIds does not exist on the object anymore.
+    $this->assertFalse(isset($object->_serviceIds));

Thhis could be either avoided, or something like assertEmpty?

6).

+  /**
+   * {@inheritdoc}
+   *
+   * Make the property accessible for the test.
+   */
+  public $_serviceIds;

This property has alreay existed:) Maybe set it public in following code to aviod declaration again?

+abstract class DependencySerialization {
+
+  /**
+   * An array of service IDs keyed by property name used for serialization.
+   *
+   * @var array
+   */
+  protected $_serviceIds = array();

As @yched mentioned at #69, point 5), I agree here should use $container->get('test_service') as paramter injected into TestClass. Otherwise, the mocked $container is useless here.

3). As mentioned above,
+ $service->_serviceId = 'test_service';
should probably be deleted and it should be tested after service gets injected. This property _serviceId will be assigned value at $container->get(), which is exactly what we create a new container for:

Here is an extra test file, I forgot yesterday evening, note: we do a unit test, so we should just test the dependency serialization not the container.

I guess code at #70 has done it properly, i.e. deleting it and testing it,?

I can't see, how this should work without the code you mentioned.

Perhaps emptying, i.e., $this->_serviceIds = array(); is a better idea over unset? "_serviceIds" is an intrinsic property of abstract class DependencySerialization other than some arbitrary property assigned during its life cycle. And the default value has been assigned an empty array. I guess it's better to keep consistency here, which is the same case at the beginning of __sleep().

I unsetted it because I though that people should just not even try to rely on the existance of it, what about setting it to NULL by default?

This property has alreay existed:) Maybe set it public in following code to aviod declaration again?

Let's not sacrifice actual running code just to make tests a bit easier.

- Yeah, sorry, my bad - $dependencySerialization = new TestClass($container->get('test_service', 'example'); cannot work, since the test uses a mock container, so Drupal\Core\DependencyInjection\Container::get() does not actually run and $_serviceId is not automatically assigned.
And this is fine - one test for the behavior of serialize/unserialize($object_with_dependencies) wrt $_serviceIds, one test for the behavior of the container wrt $_serviceId.

- +1 on keeping _serviceIds protected and default to NULL in the property definition, and unsetting it back to the same NULL value at the end of __wakeup().

- Sad panda on last patch switching back to _serviceIDs though :-).

- Nitpick: DependencySerializationTest::getInfo()
'name' => 'Service dependency serialization.',
should have no trailing dot.
(same in ContainerTest)

- Nitpick: ContainerTest & DependencySerializationTest
+ $service->key = 'value';
This doesn't seem to serve any purpose, we now check assertSame() anyway.

oops, crosspost with testbot :-/

Ha, I really got confused that we switched to ID.

Fixed all your points, let's see whether this broken test is reproducible.

Yay, looks good and passes locally :-).
This works for me.

#79: serialize-2004282-79.patch queued for re-testing.

Ha!

-    $this->_serviceIDs = array();
+    $this->_serviceIds = array();

This tells it's really necessary to empty _serviceIds in the beginning of __sleep:)

Hah :-)

Minor change.

It's not great that we have to do this, but I can't think of a better solution and what's done here is very readable/predicatlbe. Possibly some other class which can't inherit from this one might need those methods, but then it can copy and paste, or we can add a trait later in core or contrib.

So... committed/pushed to 8.x, thanks!

or we can add a trait later in core or contrib

Just wondering - does this mean we are possibly considering requiring PHP 5.4 before release ?

We could add a trait without a PHP 5.4 requirement, by not using it in core.

This would be a great idea in general though I am wondering whether the amount of people who complain that they there are things in core which does not work with 5.3

Follow-up: #2142515: Provide a method of DI for entities.