Fix leaky and brittle container serialization solution

It took about 6 readings to make sense of it but I think this makes sense. Pushing properties on to objects probably isn't the best way to do things anyways.

@chx Is this 5.4 and higher?

Plus 1

@chx I thought my question was silly as soon as I typed it.

I learned something new today!

Here is a benchmark to see whether | how much this improves things:

=== 8.0.x..8.0.x compared (55a20404acc7e..55a2048e676cb):

ct  : 31,693|31,693|0|0.0%
wt  : 72,797|72,652|-145|-0.2%
mu  : 16,746,536|16,746,536|0|0.0%
pmu : 16,800,352|16,800,352|0|0.0%

=== 8.0.x..2531564 compared (55a20404acc7e..55a2049936f3c):

ct  : 31,693|31,701|8|0.0%
wt  : 72,797|72,265|-532|-0.7%
mu  : 16,746,536|16,602,960|-143,576|-0.9%
pmu : 16,800,352|16,657,096|-143,256|-0.9%

---
ct = function calls, wt = wall time, cpu = cpu time used, mu = memory usage, pmu = peak memory usage

### XHPROF-LIB REPORT

+-------------------+------------+------------+------------+------------+------------+
| namespace         |        min |        max |       mean |     median |       95th |
+-------------------+------------+------------+------------+------------+------------+
| Calls             |            |            |            |            |            |
|                   |            |            |            |            |            |
| 2531564           |     31,701 |     34,361 |     31,728 |     31,701 |     31,701 |
| 8_0_x             |     31,693 |     33,993 |     31,716 |     31,693 |     31,693 |
|                   |            |            |            |            |            |
| Wall time         |            |            |            |            |            |
|                   |            |            |            |            |            |
| 2531564           |     72,265 |    102,852 |     75,836 |     74,694 |     82,105 |
| 8_0_x             |     72,652 |    104,095 |     76,345 |     75,276 |     82,722 |
|                   |            |            |            |            |            |
| Memory usage      |            |            |            |            |            |
|                   |            |            |            |            |            |
| 2531564           | 16,602,912 | 17,155,888 | 16,647,232 | 16,602,960 | 16,996,792 |
| 8_0_x             | 16,746,536 | 17,295,648 | 16,799,315 | 16,746,536 | 17,099,089 |
|                   |            |            |            |            |            |
| Peak memory usage |            |            |            |            |            |
|                   |            |            |            |            |            |
| 2531564           | 16,656,952 | 17,337,896 | 16,702,663 | 16,657,096 | 17,051,120 |
| 8_0_x             | 16,800,352 | 17,476,320 | 16,854,471 | 16,800,352 | 17,153,112 |
|                   |            |            |            |            |            |
+-------------------+------------+------------+------------+------------+------------+

Note: I ran this multiple times and see improvements between 0.5% and 1%, so certainly some improvements.
Also one less % of memory is also interesting.

+1, I've been meaning to create this issue ever since the serialization issue was fixed.

Added #2389193: Provide a way to determine whether a given instance was retrieved from the container which seemed to try to solve the same problem.

Thank you chx for getting rid of the static!

+++ b/core/lib/Drupal/Core/DependencyInjection/DependencySerializationTrait.php
@@ -25,20 +25,36 @@
+    if (isset($container) && $container instanceof SleepHelperInterface) {
+      $hashes = $container->getHashes();
+      foreach ($vars as $key => $value) {
+        if (is_object($value)) {
+          $service_id = FALSE;
+          // Special case the container.
+          if ($value instanceof ContainerInterface) {
+            $service_id = 'service_container';
+          }
+          else {
+            $hash = spl_object_hash($value);
+            if (isset($hashes[$hash])) {
+              $service_id = $hashes[$hash];
+            }
+          }
+          if ($service_id) {
+            // 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] = $service_id;
+            unset($vars[$key]);
+          }
+        }

Seems all of this can move into the try {} block, no? Keep the code together, move the exceptional cases to the end. (Even if in this case the exceptional case is treated as a ¯\_(ツ)_/¯ ).

+++ b/core/lib/Drupal/Core/DependencyInjection/SleepHelperTrait.php
@@ -0,0 +1,39 @@
+  public function getHashes() {
+    foreach ($this->services as $id => $service) {
+      if (is_object($service)) {
+        $this->hashes[spl_object_hash($service)] = $id;
+      }
+    }

If we always overwrite the hashes, why would we need to set them?

Also if we persist the object from one container to another, why would it change?

We do not override on rebuild -- that's why we index with hash and not id. Let's see an example when rebuild happens: on Container you have AccessManager(1) which gets referenced from ModuleListForm->accessManager so when Container gets destructed in rebuild the AccessManager(1) object doesn't go away. Now after rebuild ContainerBuilder holds AccessManager(2) so now you have $hashes[hash(AccessManager(1))] = access_manager and you have $hashes[hash(AccessManager(2))] = access_manager too and you need both.

Don't we want to add some documentation which explains that into the patch itself?

1. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
   @@ -1035,21 +1032,4 @@ public function alreadyImporting() {
   -  /**
   -   * Gets all the service dependencies from \Drupal.
   -   *
   -   * Since the ConfigImporter handles module installation the kernel and the
   -   * container can be rebuilt and altered during processing. It is necessary to
   -   * keep the services used by the importer in sync.
   -   */
   -  protected function reInjectMe() {
   -    $this->_serviceIds = array();
   -    $vars = get_object_vars($this);
   -    foreach ($vars as $key => $value) {
   -      if (is_object($value) && isset($value->_serviceId)) {
   -        $this->$key = \Drupal::service($value->_serviceId);
   -      }
   -    }
   -  }
   -
   

   Nice!! One less hack

2. +++ b/core/lib/Drupal/Core/DependencyInjection/SleepHelperInterface.php
   @@ -0,0 +1,21 @@
   +
   +
   

   Nitpcik: 2 lines

3. +++ b/core/lib/Drupal/Core/DependencyInjection/SleepHelperInterface.php
   @@ -0,0 +1,21 @@
   +
   +interface SleepHelperInterface {
   +
   

   Needs some helpful docs. Maybe some high level overview.

4. +++ b/core/lib/Drupal/Core/DependencyInjection/SleepHelperTrait.php
   @@ -0,0 +1,39 @@
   + * @file
   + * Contains
   + */
   

   Missing FQCN

5. +++ b/core/tests/Drupal/Tests/Core/DependencyInjection/ContainerTest.php
   index ad05400..bd7fe01 100644
   --- a/core/tests/Drupal/Tests/Core/DependencyInjection/DependencySerializationTest.php
   
   --- a/core/tests/Drupal/Tests/Core/DependencyInjection/DependencySerializationTest.php
   +++ b/core/tests/Drupal/Tests/Core/DependencyInjection/DependencySerializationTest.php
   

   It is pretty cool that this continues to work!

While I like the direction this is going, I don't think reinjectMe can be removed that easily.

It might be we have changed things elsewhere, so it is no longer necessary, but originally it was necessary and I can't see how the hashing helps here to avoid ConfigImporter getting out of sync.

I'm all for removing _serviceId, introducing that was a terrible idea in the first place.

Now after rebuild ContainerBuilder holds AccessManager(2) so now you have $hashes[hash(AccessManager(1))] = access_manager and you have $hashes[hash(AccessManager(2))] = access_manager too and you need both.

This looks like a lot of trouble, we must get rid of all situations where multiple instances of one service are used (#2282371: Document the fact that it is not safe to reuse services across container rebuilds). Therefore removing reinjectMe is not a good idea - even if tests pass. In fact the test failures in #2389193: Provide a way to determine whether a given instance was retrieved from the container indicate where we still have issues with services being reused across container rebuilds.

I presume some services are holding state which gets lost when rebuilt but we have added the persist service tag just for that purpose very long ago.

The persist tag turned out to be a terrible idea also: #2190665: Remove persist flag from services that do not need it, #2285621: Remove persist-tag and replace it with a container.state_recorder service, #2368263: Remove the persist service tag

In most instances the problems do not stem from state being lost during container rebuilds but from obsolete state being reused. For example entity manager and field definitions, maybe plugins also caused problems in the past because obsolete state was reused after enabling a module.

Also related #2260187: [PP-1] Deprecate drupal_static() and drupal_static_reset() I don't think any class should use static:: that is not resettable and any persisted service should have a clean up interface.

Overall looks great!

1. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
   @@ -1043,13 +1043,8 @@ public function alreadyImporting() {
   +    $this->__sleep();
   +    $this->__wakeup();
   

   lol :)

2. +++ b/core/lib/Drupal/Core/DependencyInjection/DependencySerializationTrait.php
   @@ -25,22 +25,38 @@
   +              // 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] = $service_id;
   +              unset($vars[$key]);
   

   Should we not store _serviceIds in $vars?

   That feels a little strange ...

   Would that also break the property optimization?

3. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -755,6 +755,8 @@ protected function initializeContainer() {
   +      // Save the current hashes.
   +      $hashes = $this->container->getHashes();
   

   Is it possible to only do this on a rebuild?

   Or do we do that already? (Could be from the code indentation).

   I now finally understood that something might still reference an old service of an outdated container, but because hashes are propagated we keep that information.

   However need to carefully measure memory, with our webtests could easily lead to a kinda memory leak.

   Also might want to profile this at least a little. I have no idea how fast or not fast spl_object_hash() is.

   We have lots of services initialized on container rebuild time, so it depends on how fast that is.

4. +++ b/core/modules/system/src/Form/ModulesListForm.php
   @@ -103,6 +103,13 @@ class ModulesListForm extends FormBase {
      /**
   +   * The access manager.
   +   *
   +   * @var \Drupal\Core\Access\AccessManagerInterface
   +   */
   +  protected $accessManager;
   

   This was just missed before I assume?

Thanks for answering my feedback.

This is RTBC :).

+++ b/core/lib/Drupal/Core/DependencyInjection/SleepHelperTrait.php
@@ -0,0 +1,39 @@
+        $this->hashes[spl_object_hash($service)] = $id;

How about SplObjectStorage?

I was wrong ; sorry ; the SplObjectStorage does have a runaway memory problem -- storing just the hashes instead of the objects solves it.

Oh yeah that makes sense.

Yeah that does make sense, shame the code was cleaner but alas, +1

1. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
   @@ -1043,13 +1043,8 @@ public function alreadyImporting() {
   +    $this->__sleep();
   +    $this->__wakeup();
   

   I doubt that this will work when a module swaps out a class of a service injected into the config importer. E.g., if a module provides a new event dispatcher, then the service id of the injected one will not be detected because __sleep operates on the new container. This will result in different object hashes for the old injected service vs. the new one from the container.

   Also, I guess that the service in the new container might not have been initialized yet(?) As a result sleep helper wouldn't return any hash for it.

   In order to make this work, the hashes need to be collected before the container is rebuilt (or alternatively from the old container).

2. +++ b/core/lib/Drupal/Core/DependencyInjection/SleepHelperInterface.php
   @@ -0,0 +1,21 @@
   +interface SleepHelperInterface {
   

   Documentation missing.

3. +++ b/core/lib/Drupal/Core/DependencyInjection/SleepHelperTrait.php
   @@ -0,0 +1,39 @@
   +/**
   + * @file
   + * Contains
   + */
   

   What?

4. +++ b/core/lib/Drupal/Core/DependencyInjection/SleepHelperTrait.php
   @@ -0,0 +1,39 @@
   +trait SleepHelperTrait {
   

   Should have a more specific name which makes it clear, that this is container internals. E.g. ContainerSleepHelperTrait.

#46.1: That is what is happening.

It is non-obvious and it took me a while, but that indeed works that way.

I am still happy with #28.

I fear that I do not quite understand #47. Are you saying that it is acceptable to work with outdated services? Or do you think that what I'm proposing here is already implemented and thus my concerns are invalid?

So to be fair this is at least not RTBC at the moment.

+++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
@@ -1043,13 +1043,8 @@ public function alreadyImporting() {
   protected function reInjectMe() {
...
+    $this->__sleep();
+    $this->__wakeup();

This should at least explain how this exactly works / vs. the limitations as described in #47.1

Instead of this can we simply define and apply to classes used as services a trait that defines that class property so as to avoid breaking the optimization?

Instead of this can we simply define and apply to classes used as services a trait that defines that class property so as to avoid breaking the optimization?

It is not only that, its also that contrib/custom could easily miss that.

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -755,6 +774,8 @@ protected function initializeContainer() {
+      // Save the current hashes.
+      $this->getServiceIdMapping();

+++ b/core/lib/Drupal/Core/DrupalKernelInterface.php
@@ -134,4 +134,9 @@ public function preHandle(Request $request);
+   * Get a mapping from service hashes to service IDs.
+   */
+  public function getServiceIdMapping();

why getter saves any hashes?
maybe better change name of method?

let's agree on trying very, very hard not to put phpwtf.org worth code in runtime. Tests are different.

As far as I can tell we wouldn't need that if the mapping would be stored in its own service instead of inside the kernel. So maybe clean test code in addition to separation of concerns (as pointed out in IRC yesterday) is reason enough to extract that functionality?

It does not make sense to close this issue because it has more substantial conversation and more followers than the other one.

I would still like to see this issue going somewhere :(

Agreed. Here is a naive re-roll of what chx had done previously to get it applying. Only change from the original patch is removing @file from new file and removing a dead use from one of the files related to code removed in the patch. I did it in the merge commit though so I don't have a interdiff. Sorry.

Haven't really done a review of it other then confirming the patches looked similar, installing drupal, and clicking around and confirming things worked.

Another reroll. Trying to figure out what the failure in search still because I'm having trouble recreating it.

#2380293: Properly inject services into ModuleInstaller unfortunately started using this non-api and needs to be unraveled now before this can be committed.

Ok now that is weird...
Fatal error: Call to undefined method Drupal\Core\PathProcessor\PathProcessorAlias::getSourceStorage() in /var/www/html/core/lib/Drupal/Core/Config/ConfigImporter.php on line 784

That line:

    \Drupal::service('config.installer')
      ->setSourceStorage($this->storageComparer->getSourceStorage());

So clearly that's not suppose to be a PathProcessorAlias. Its like spl_object_hash() is colliding somehow and we're getting the wrong object in random places. But that can't be possible because we type hint service injection... So the only way I can see that happening is if there are some reference wonkyness replacing the internal object.

The documentation does document a possible collision case:

When an object is destroyed, its hash may be reused for other objects.

That seem OK for our purposes really and the object clearly isn't destroyed if its being used right? So... what's going on?

Discussed with @effulgentsia, @catch, @xjm, @cilefen. We decided to mark #2536370: DependencySerializationTraitPass does not add _serviceId to decorated services as a duplicate of this issue. This issue should include a test for that bug. We agreed that given the nature of that bug and performance improvements hinted at by #9 that this issue is a major bug.

The #2536370: DependencySerializationTraitPass does not add _serviceId to decorated services bug is pretty nasty to debug allow there is a workaround - on the decorated service you can set the serviceId manually in the service definition see #2536370-7: DependencySerializationTraitPass does not add _serviceId to decorated services

I personally consider this blocked on drupalci changes since the current errors seem specific to an unpatch spl_object_hash() behaving in a way other then its documentation in an unpatched 5.5.23. That or we implement our own version but I'm not sure we can. I don't know if we need to bump the php version or how to handle the specifics of treating and resolve this.

Just kinda a blind reroll to see what testbot thinks.

Re rolled.

Re rolled.

The last two patches are incomplete, as they're missing the IntrospectableContainerInterface.

The IntrospectableContainerInterface actually was already deprecated and is removed in 3.0, so we should be extending the ContainerInterface instead.

Note that replacing DependencySerializationTraitPass with this solution, which gets the service ID mapping from the live container, coincidentally fixes #2896993: Decorated services crash on serialization., because the compiler pass currently runs before decorators are resolved, so overridden services have incorrect (or missing) IDs.

If I'm understanding the changes correctly, then the DependencySerializationTest now uses an instance of the Symfony container class, rather than the Drupal one. The Symfony class hardcodes service_container to $this and actively blocks it being assigned; therefore the test fails.

I've replaced the set() command with a sanity check that asserts get('service_container') returns the container itself.

Yes, that ContextualFiltersBlockContextTest failure is exactly what it looks like: A TranslateableMarkup object gets picked up by the DST and somehow mistakenly identified as a service.

(
    [label] => drupal.proxy_original_service.plugin.cache_clearer
)

WTF.

Added the following to Drupal\Core\Plugin\Context\ContextDefinition:

  public function __sleep() {
    $mapping = \Drupal::service('kernel')->getServiceIdMapping();
    assert(FALSE,
      $this->label . "\n" .
      spl_object_hash($this->label) . "\n" .
      spl_object_hash(\Drupal::service('plugin.cache_clearer')) . "\n" .
      $mapping[spl_object_hash($this->label)] . "\n" .
      $mapping[spl_object_hash(\Drupal::service('plugin.cache_clearer'))]
    );
    return parent::__sleep();
  }

Got out the following:

000000007c5c204f0000000007b14bfc
000000007c5c23610000000007b14bfc
drupal.proxy_original_service.plugin.cache_clearer
drupal.proxy_original_service.plugin.cache_clearer

These two hashes are 0x32e0000000000000000 apart and yet they return the same array value.

It appears that one of the many hashes mapped to the plugin.cache.clearer service collides with this particular translateable markup object, and what is more, does so in a consistent fashion (see the attached text file for a dump of all hashes and service IDs at the time of the collision).

I'm not sure why there are so many hashes, by the way. spl_object_hashes guarantees "A string that is unique for each object and is always the same for the same object."

Unless the container deinits and reinits the same service multiple times (up to sixteen for some, such as the cache_tags invalidator), the second condition appears to be violated. (And if it does reinstantiate the same service, then why does it do so?)

The first condition appears to be definitely violated considering we're getting this collision.

Emptying the service mapping every time seems like it fixes this specific case, but it doesn't solve the underlying mystery.

And in particular, if a service can be instantiated multiple times, then we can't actually rely on the mapping. By the time an object is serialized, its injected service might not match the service registered in the container anymore.

Rebuilding this map with every serialization might be too much of a performance drag. Which is kind of ironic.

Maybe it would be better to update the map from within the container whenever a service gets instantiated?

From the docs:

This id can be used as a hash key for storing objects, or for identifying an object, as long as the object is not destroyed. Once the object is destroyed, its hash may be reused for other objects.

I suspect this entire approach is broken. We can't identify services by their SPL hash, because it's possible to reset the container. At which point

1) any service that is still injected somewhere is orphaned (the container will get a fresh instance when it's next required). If we clear the mapping, we lose its hash.

2) any service that is not in use gets GCd, and if we don't clear the mapping, we may find its old hash now being used by a completely different object.

So why are we using spl here? It would be more reliable to only use the class name. The only consequence would be picking up instances of the class that didn't come from the container - an edge case that the spl hash also couldn't quite handle:

Because of this hash passing the approach is not 100% bulletproof: the hash depends on name of the class and the number of instances alive for a class. So theoretically it is possible that a Foo instance lived in the Container, then the container was rebuilt and now both Container and this Foo instance are gone and the class using a DependencySerializationTrait creates a new Foo object which is not a service.

For extra safety, we could concatenate both class name and spl - provided we ensure that we never get the case where a service is initialized, injected and deinitialized without ever being mapped.

New patch.

I kind of suspect the problem of non-service instances of a service class is quite negligible. Classes that use the DependencySerializationTrait already have so many pitfalls to deal with - private variables that get lost, non-service variables that store services and don't use DependencySerializationTrait, other variables that might not be serializable for weird reasons...

By comparison, a rule that if you put your own instance of a service class in a non-private variable, serialization&disinterment will replace it with an instance pulled from the container, is fairly straightforward.

So why are we using spl here? It would be more reliable to only use the class name.

We where using spl because class doesn't tell us enough to map to a specific service. It is reasonable to have the same class in different places on the container. I see ViewsPluginManager registered 12 times for different services and getting the wrong one is certainly going to cause problems and all the other instances will not be properly replaced.

Lets give this a shot. Its not pretty but I'm curious what the outcome is.

So.. it says clearly two failures but that's one failure. And of course something started relying on the serviceid property we're trying to kill :( :(

#2849674: Complex job in ViewExecutable::unserialize() causes data corruption

Hopefully we can just convert this to the service map on the kernel. :pray:

+++ b/core/modules/views/src/ViewExecutable.php
@@ -2488,8 +2489,8 @@ public function __sleep() {
+      'views_data' => \Drupal::service('kernel')->getServiceIdMapping()[DrupalKernel::generateServiceIdHash($this->viewsData)],
+      'route_provider' => \Drupal::service('kernel')->getServiceIdMapping()[DrupalKernel::generateServiceIdHash($this->routeProvider)],

+++ b/core/tests/Drupal/Tests/Core/DrupalKernel/DrupalKernelTest.php
@@ -192,6 +195,25 @@ public function testFindSitePath() {
+      $this->assertEquals($container->get('kernel')->getServiceIdMapping()[DrupalKernel::generateServiceIdHash($service)], 'bar');
...
+      $this->assertNotEquals(DrupalKernel::generateServiceIdHash($service1), DrupalKernel::generateServiceIdHash($service2));

maybe move that to some helper reusable for contrib?

I thought about it and am not sure what an interface for the helper would look like or where it would live.

The current interface is concise, there is an array of service id's keyed by a hash of the class. There are public methods for getting that hash and creating the hash. It'd be more concise if it wasn't for the PHP quirk but it isn't the first or last quirk we'll work around.

One final note, contrib _really_ shouldn't be digging around in the map in the first place. Its there for core magic(the container serialization logic) and if you're doing something ugly well, the rope is there to hang yourself with but we don't need to make it pretty at the cost of complexity to interfaces. I would have honestly refactored the views code so it wasn't even used but it wasn't immediately obvious to me what it was doing and that made it out of the scope of what this and easier to handle in a follow up than block.

Coming back to this: Would it be feasible to instead make services implement a ServiceInterface with a setter/getter for the service ID (and a trait)? For interoperability, we could add a compiler pass that checks for services not implementing ServiceInterface and proxies them, analogously to Drupal\Core\DependencyInjection\Compiler\ProxyServicesPass.

The only problem with the current approach is setting a property that isn't statically defined. This could be solved by statically defining it, using a generated wrapper class if necessary.

I still have the impression that the array idea is too magical. As we've already seen, it comes with serious pitfalls - the original approach used only the internal pointers (spl_object_hash), and even the new approach only mitigates this by also using the class name.

Its been a while since I've looked at this but memory serves we can't rely on classes to do it for us because the container builder needs to know thing about and cache all services.

Re-roll of the current solution. Just fixes a use conflict.

#2935503: Remove rest.module's use of _serviceId for module resolution to resolve rest.module's use.

The inability to correctly "serialize" decorated services makes it impossible to use together the modules autosave_form and entity_embed - #2980855: Entity Embed breaks if Autosave form is active, which now officially makes the issue a contributed project blocker.

blocked by #2935503: Remove rest.module's use of _serviceId for module resolution

Curious if the issue title/issue summary could use a refresh. The launch of 8.7.0 will see the removal of support for php 5.x, so if this is a 5.x specific issue, I cant see why it'd be fixed. But if this is still an issue for 5.x all the way to 7.3, then maybe the title shouldnt say "5.4" in it.

Honestly with the current implementation I'm not sure the we're pushing any definable performance gains. Or more accurately, it probably does produce performance improvements in general usage because objects on the container are slightly smaller, there's a dropped function call on container set, and the implementation is now entirely container to the sleep/wake process. But they are small enough in 7.X it is going to difficult to measure them.

Mostly the current implementation pollutes classes with random properties, leaks implementation details, and generally causing problems and that should be fixed.

I've tried to update the title and summary to reflect that.

Bonus, a reroll to fix a use conflict. We can still discuss the implementation/review while the Rest issue blocks actually passing tests.

Reroll. Includes the rest fixes because its just not moving and without it its not really possible to meaningfully review this task. Also includes some other tests that have been added in the past year.

There where some conflicts so the interdiff isn't perfect but it has the meaningful changes.

While reading the patch I'm wondering about the following things:

1. This doesn't resolve the general weirdness that we need this crazy rehydration
2. This increases the surface area quite a bit: Previously we had mostly just two places, now we have 3 places plus a public API. One thing I see directly is that the kernel and the container is aware of things now. Is this something which could be improved? Could we move getServiceIdMapping into the service container.

Here is some nitpick review:

1. +++ b/core/lib/Drupal/Core/DependencyInjection/ContainerInterface.php
   @@ -0,0 +1,19 @@
   +   *
   +   * @return array An array of all defined service ids
   +   */
   

   Nitpick: This doesn't comply with our documentation standard. The comment should be on the next line

2. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -827,6 +832,24 @@ public function updateModules(array $module_list, array $module_filenames = [])
   +    return hash('sha256', get_class($object) . spl_object_hash($object));
   

   I'm curious how get_class makes this more unique. Is there a reason for this? Maybe there could be an inline comment about that.

Yeah, we still have goofy rehydration. I'm not sure it increases the surface area as creates a public API for something that was occasionally (at least in rest.module) used like one. I'll see if it can fit on the container without getting... recursively terrible though.

1) eww... yes.
2) yeah, there is some history hidden in the 100s of previous comments but its because spl_object_hash is not unique in time but entirely unique. @see #73 and this bit of documentation on spl_object_hash "When an object is destroyed, its hash may be reused for other objects." Basically we try to re-hydrate and end up with a different type of object and everything dies. In reality... I'm not sure we'd ever run into this on a real site, there would likely have to be a lot of rebuilding, object destruction, and gc. That is exactly what happens in our test suite though and we just got miles of random brokenness. Narrowing it by class name made things "good enough".

Well at least good enough in previous tests to make things work. I can't recreate the failures in the last patch(if they are failures... they show passes and claim failures) so I'm going to have to do some deep digging and hopefully this isn't the cause again...

2) yeah, there is some history hidden in the 100s of previous comments but its because spl_object_hash is not unique in time but entirely unique. @see #73 and this bit of documentation on spl_object_hash "When an object is destroyed, its hash may be reused for other objects." Basically we try to re-hydrate and end up with a different type of object and everything dies. In reality... I'm not sure we'd ever run into this on a real site, there would likely have to be a lot of rebuilding, object destruction, and gc. That is exactly what happens in our test suite though and we just got miles of random brokenness. Narrowing it by class name made things "good enough".

Wow! Thank you for the explanation! I think it would be really valueable to document this in the code.

Referenced this issue in #3056604: Add/remove event listeners provided by a module on module install/uninstall.

Yeah, we're still adding these... I guess I'll pick this up and add some more issues to remove the other uses though I feel like no one cares anymore.

I guess its not actually that bad.

- reroll
- removed #2935503: Remove rest.module's use of _serviceId for module resolution from the patch since its not relevant anymore
- optimistically updated version numbers in deprecations.
- Added a comment re #131

woops, I um... didn't include the interdiff changes in the patch.

Here's the patch that should have been in 134 for posterity plus an updated patch to fix for the way deprecation where being tested.

Ok, this should pass. It looks like I got a little flustered a few patches ago with testing failures and didn't quite fix the ConfigEntityBaseUnitTest correctly. Building a container and kernel in unit tests is gnarly but I guess its not "a fully functioning kernel" so we do it pretty often and don't need to move that test to the kernel suite.

Anyways this should be pretty solid again. That only thing that stood out to me skimming that patch again was that _maybe_ these service mapping kernel methods should be tagged @internal. They are still not really designed as an API for application development but hacky bits to resolve this serialization problem so there would be some logic in doing it but I don't think its strictly necessary.

Reviewed and tested patch 136, seems to work fine and is even slightly faster.
I have two comments:

• The change to ContainerInjectionInterface is not really needed
• ViewExecutable getServiceIdMapping array can be reused

Uploaded a new patch with above two comments fixed.

Bug in the ether it seems... green now :).

Thanks for the review!

You're right,the ContainerInterface alias isn't strictly necessary. It's been 3 years since I added it but I'm pretty sure my intention was to make the distinction from the ContainerInterface in the same namespace explicit to developers where the interaction with the use might not be obvious. (shrug)

If your this deep into the workings of Drupal you know how to read namespaces and know that Drupal depends on the Symfony framework. I don't see any upside to this alias to be honest.

RTBC then I guess, agreed?

I am giving this a RTBC, it looks good and if it is merged, I can probably finalize #3056604: Add/remove event listeners provided by a module on module install/uninstall.

1. I like this change. The container mutating the state of container services was a pragmatic solution but this solution is better.

2. +++ b/core/lib/Drupal/Component/DependencyInjection/Dumper/OptimizedPhpArrayDumper.php
   @@ -46,6 +46,8 @@ class OptimizedPhpArrayDumper extends Dumper {
   +  protected $aliases = [];
   +
   

   Unrelated change.

3. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -819,6 +824,29 @@ public function updateModules(array $module_list, array $module_filenames = [])
   +    // Include class name as an additional namespace for the hash since
   +    // spl_object_hash's return can be recycled. This still is not a 100%
   +    // guarantee to be unique but makes collisions incredibly difficult and even
   +    // then the interface would be preserved.
   +    // @see https://php.net/spl_object_hash#refsect1-function.spl-object-hash-notes
   +    return hash('sha256', get_class($object) . spl_object_hash($object));
   

   Why do have to hash here? Couldn't we do get_class($object) . spl_object_hash($object)

   Does the hashing buy us anything over than shorter keys which are meaningless and more compute time?

4. +++ b/core/tests/Drupal/Tests/Component/DependencyInjection/Dumper/OptimizedPhpArrayDumperTest.php
   @@ -8,6 +8,7 @@
   +  use Drupal\Tests\Traits\ExpectDeprecationTrait;
   
   @@ -24,6 +25,8 @@
   +    use ExpectDeprecationTrait;
   +
   

   I think this is not necessary.

5. +++ b/core/tests/Drupal/Tests/Core/Plugin/ContextHandlerTest.php
   @@ -63,7 +64,7 @@ protected function setUp(): void {
   -    $container = new ContainerBuilder();
   +    $container = TestKernel::setContainerWithKernel();
   

   This change is tricky from a BC perspective.

6. Some coding standards stuff
   

   FILE: /Users/alex/dev/sites/drupal8alt.dev/core/lib/Drupal/Core/DependencyInjection/Compiler/DependencySerializationTraitPass.php
   -----------------------------------------------------------------------------------------------------------------------------------------------------
   FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
   -----------------------------------------------------------------------------------------------------------------------------------------------------
    1 | WARNING | No PHP code was found in this file and short open tags are not allowed by this install of PHP. This file may be using short open tags
      |         | but PHP does not allow them.
   -----------------------------------------------------------------------------------------------------------------------------------------------------
   
   FILE: /Users/alex/dev/sites/drupal8alt.dev/core/lib/Drupal/Core/DrupalKernel.php
   --------------------------------------------------------------------------------
   FOUND 1 ERROR AFFECTING 1 LINE
   --------------------------------------------------------------------------------
    831 | ERROR | [x] Separate the @param and @return sections by a blank line.
   --------------------------------------------------------------------------------
   PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
   --------------------------------------------------------------------------------
   
   FILE: /Users/alex/dev/sites/drupal8alt.dev/core/tests/Drupal/Tests/Core/DependencyInjection/DependencySerializationTest.php
   ---------------------------------------------------------------------------------------------------------------------------
   FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
   ---------------------------------------------------------------------------------------------------------------------------
    10 | WARNING | [x] Unused use statement
   ---------------------------------------------------------------------------------------------------------------------------
   PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
   ---------------------------------------------------------------------------------------------------------------------------
   
   FILE: /Users/alex/dev/sites/drupal8alt.dev/core/tests/Drupal/Tests/Core/Entity/EntityTypeTest.php
   -------------------------------------------------------------------------------------------------
   FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
   -------------------------------------------------------------------------------------------------
    10 | WARNING | [x] Unused use statement
   -------------------------------------------------------------------------------------------------
   PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
   -------------------------------------------------------------------------------------------------
   
   FILE: /Users/alex/dev/sites/drupal8alt.dev/core/tests/Drupal/Tests/Core/Plugin/ContextHandlerTest.php
   -----------------------------------------------------------------------------------------------------
   FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
   -----------------------------------------------------------------------------------------------------
    19 | WARNING | [x] Unused use statement
   -----------------------------------------------------------------------------------------------------
   PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
   -----------------------------------------------------------------------------------------------------
   
   FILE: /Users/alex/dev/sites/drupal8alt.dev/core/tests/Drupal/Tests/Core/TempStore/SharedTempStoreTest.php
   ---------------------------------------------------------------------------------------------------------
   FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
   ---------------------------------------------------------------------------------------------------------
    10 | WARNING | [x] Unused use statement
   ---------------------------------------------------------------------------------------------------------
   PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
   ---------------------------------------------------------------------------------------------------------
   

7. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
   @@ -1056,13 +1056,13 @@ public function alreadyImporting() {
   -    $this->_serviceIds = [];
   -    $vars = get_object_vars($this);
   -    foreach ($vars as $key => $value) {
   -      if (is_object($value) && isset($value->_serviceId)) {
   -        $this->$key = \Drupal::service($value->_serviceId);
   -      }
   -    }
   +    // When rebuilding the container,
   +    // \Drupal\Core\DrupalKernel::initializeContainer() saves the hashes of the
   +    // old container and passes them to the new one. So __sleep() will
   +    // recognize the old services and then __wakeup() will restore them from
   +    // the new container.
   +    $this->__sleep();
   +    $this->__wakeup();
   

   I've been working on a more event driven config importer that will hopefully reduced what's being done here. But atm it still needs it and this implementation is neat.

8. +++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/DependencySerializationTraitPass.php
   @@ -1,28 +0,0 @@
   -class DependencySerializationTraitPass implements CompilerPassInterface {
   

   I've been thinking about the removal of this class. Class removal often goes wrong. I think we should deprecate it and make it a no-op here. And remove in D10.

9. +++ b/core/modules/views/src/ViewExecutable.php
   @@ -2488,10 +2489,11 @@ public function getDependencies() {
   -      'views_data' => $this->viewsData->_serviceId,
   -      'route_provider' => $this->routeProvider->_serviceId,
   +      'views_data' => $serviceIdMapping[DrupalKernel::generateServiceIdHash($this->viewsData)],
   +      'route_provider' => $serviceIdMapping[DrupalKernel::generateServiceIdHash($this->routeProvider)],
   

   We definitely need a well written change record for this. I was wondering if there was a way to do this in a more BC compatible way. But I don't think there is. In an ideal world we could set the _serviceId to a function that triggers a deprecation if used. And somehow support the existing functionality and then remove it in D10. Looking at http://codcontrib.hank.vps-private.net/search?text=_serviceId&filename= shows that this change is definitely going to impact contrib :(

Does the hashing buy us anything over than shorter keys which are meaningless and more compute time?

No reason in practice; afaict it's only used as an array key so there is no length limitation.

Though the function *is* named generateServiceIdHash (not "generateServiceIdentifier" or something), so if it returns a variable-length unique string instead of a fixed-length hex string, it wouldn't be quite what it says on the tin.

This one addresses 2), 4), 6) and 8) of #144.

There's at least one outstanding review I don't think has really been addressed which kept me from RTBCing this.
from #129

This increases the surface area quite a bit: Previously we had mostly just two places, now we have 3 places plus a public API. One thing I see directly is that the kernel and the container is aware of things now. Is this something which could be improved? Could we move getServiceIdMapping into the service container.

I did this before and went down the wrong rabbit whole and hadn't come back. I've started on this again last week and got something that's looking usable but there are some weird failures in the kernel test suite I'm trying to track down. Before posting it for discussion. The tradeoff is relying more on the Drupal Container interface instead of the Kernel which might make this a _little_ bit bigger change but maybe better.

Thanks for the review Alex! Sounds close though... addressing Daniel's concerns might shake that up... Hopefully not.
1) 100%
2) It was related to the original scope of fixing undefined properties but its a quick fix i guess. :-/
3) I think cburschka is right, originally it was just he spl hash then we needed to separate them. I think I _might have run into some key length issues but that should be validated. I'm not sure that large keys are free though there's probably a tradeoff we should quantify and make a decision on. I'll look into it some more.
4) It its not needed, that's awesome! :-D
5) I'd like to know more about your thoughts here. Its in testing so less BC promise and I think the difference was just that the Builder ended up with a kernel on it since this relied on this new part of the kernel. Its not ideal but its a useful wrapper for that pattern even if the method name is super confusing. That said the container version of this patch I was playing with might
6) Woops...
7) Nice! I hope this simplification helps your work.
8) I'm not sure I agree since this is clearly a very internal and limited class but you've got more experience with this.
9) For sure. Though a lot of those reference this issue, are fixtures in tests, or are unmaintained/unreleased so it might be even more important that this exist for site developers where we can't measure and there is a higher likely hood of weird hacks to fix edge cases.

(Regarding #2536370: DependencySerializationTraitPass does not add _serviceId to decorated services, I've checked up on a smaller patch I submitted to fix the decorated service bug in #2896993: Decorated services crash on serialization., which still works. That might be relevant for the branches that this major change can't be applied to.)

Ok this should do it. The interface looks a little bigger on the container. I believe the intent was _not_ to do that originally but the fact that the patch shrinks quite a bit and we aren't futzing with kernels in all those tests smells a bit better I think so I think Daniel was right in that overall this ends up simplified. Notably the ContainerInterface has to move from Core to Component because now \Drupal\Component\DependencyInject\Container needs to implement it for everything to work and there is a new trait to share that logic with the container builder.

Additional changes:
- I moved the deprecation to a trigger_error and updated the format for the new standard.
- I also removed a lot of the TestKernel stuff and moved the EntityTypeTests code back to the unit test as I expected. Only small changes to get those to work now.

Sort of unrelated in EntityTypeTest I modified the exception logic in the serialize test. When I reverted it and it failed it was really hard to understand what was failing.
Before:

Exception:
/app/core/tests/Drupal/Tests/Core/Entity/EntityTypeTest.php:514
...

After:

Exception: Serialize should not be called.
/app/core/tests/Drupal/Tests/Core/Entity/EntityTypeTest.php:514
...

I tried using the "shouldNotBeCalled" prediction which seems more accurate on a mock but the error comes out really weird without line numbers or a useful trace. I think because serialize isn't an interface method but I'm not sure.

spun off #3143085: Define and optimize alias definition in OptimizedPhpArrayDumper and #3143087: Define accessManager property for ModulesListForm for another missing property definition I ran into.

bah, I clicked the wrong file I guess. here's the interdiff.

Those failures are rough. We can't install a site with this patch and I think I figured out why. What seems to happen(glossing over a lot of details because the installer is a ball of spaghetti) is this:

1) Installer bootstraps everything and sets up a container. Looking good.
2) We generate a batch array. On that array we put some Urls, each end up with references to a urlAssembler from container #1
3) Batch installs a module
4) Module installer triggers rebuild with container #2
5) We wrap up the batch and serialize the batch array(you might see where this is going now)
6) DependencySerializationTrait kicks in on those Urls from step 1. It pulls the container(#2), removes any services that match that container... which are obviously 0 since its different from what generated the Url.
7) Serialize kicks in and BOOM! there's a database connection on those services and we're dead.

The reason the kernel approach worked is that it remembers service ids across these rebuilds by storing them on the container. I should have remembered that but it was hidden in a bunch of discussions and logic that wasn't clear mostly dating back 5 years :(

Oh yeah, the mappings can't be reset if the container is rebuilt, since the old services are still injected all over the place and may still need to be mapped. I think that array effectively has to last for the rest of the PHP runtime?

Pretty much, I like how some of the things shook out in this patch but I think the resolution is going to have to go to go back into the kernel because its the only thing that really persists like this. The "collection" logic might make sense to remain. I'm going to take another pass and see how much I can salvage or if we need to scrap it and go back to #146. At the very least this can be used to document some logic and maybe add some tests to explain why things are on the kernel.

I kinda got burned out on this but it looks like it might be coming back. RFC to deprecate the functionality dynamic properties like _serviceId goes to a vote soon and that'll shake things up.

https://wiki.php.net/rfc/deprecate_dynamic_properties

As I see voting will be Yes, so it needs reroll/revamp

Yes, I think the only effect we might have is a hard push might stretch the timeline. Even the no's only seem to be worried about the timeframe. But I've not heard anyone else in the Drupal community raising alarms so unless we start hitting the mailing list I think we're going to have to deal with the outcome. The change itself is very popular and I think probably with good reason so some for is certain to get through soon.

related plan has number of issues to clean-up and cover by phpunit the missing/undefined properties

The dynamic properties RFC is approved and going into PHP 8.2, so this is definitely affected. Adding related issue which depends on this property.

Man its hard to get excited about rerolling this issue. Here it is though. I moved the collection logic down into the container like mentioned in #153 and did some general cleanups. Seems to work in local testing.

Lot of resolved conflicts but a rough approximation of an interdiff attached as well.

Not sure how to tag "this might be an interface and if it is it will 100% break during the next release cycle" but its probably bordering on critical. I guess we're all aware though so not touching it.

FILE: /app/core/lib/Drupal/Core/DependencyInjection/Compiler/DependencySerializationTraitPass.php
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
 13 | WARNING | The @see url '\Drupal\Core\DependencyInjection\DependencySerializationTrait' does not match the standard: http(s)://www.drupal.org/node/n or http(s)://www.drupal.org/project/aaa/issues/n
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------

That's confusing... after some digging through coder I found the message doesn't really have anything to do with the problem. I guess we're following deprecation annotations with a @see comment now.

Anyways, this should fix everything.

oh jeees I diffed against 10. fingers... come on.

I am running test's locally. ugh.

Here's re-roll for 9.5 and 10.0

Used to test it with PHP 8.2alpha2 and it does not fix deprecated dynamic properties https://php.watch/versions/8.2/dynamic-properties-deprecated

Log a bit cleaner but still tons of messages

[24-Jun-2022 15:55:56] WARNING: [pool app] child 218 said into stderr: "NOTICE: PHP message: PHP Deprecated:  Creation of dynamic property Drupal\sqlite\Driver\Database\sqlite\Connection::$_serviceId is deprecated in /var/www/html/web/core/lib/Drupal/Component/DependencyInjection/Container.php on line 288"
[24-Jun-2022 15:55:56] WARNING: [pool app] child 218 said into stderr: "NOTICE: PHP message: PHP Deprecated:  Creation of dynamic property Drupal\Core\Logger\LogMessageParser::$_serviceId is deprecated in /var/www/html/web/core/lib/Drupal/Component/DependencyInjection/Container.php on line 288"
[24-Jun-2022 15:55:56] WARNING: [pool app] child 218 said into stderr: "NOTICE: PHP message: PHP Deprecated:  Creation of dynamic property Drupal\dblog\Logger\DbLog::$_serviceId is deprecated in /var/www/html/web/core/lib/Drupal/Component/DependencyInjection/Container.php on line 288"
[24-Jun-2022 15:55:56] WARNING: [pool app] child 218 said into stderr: "NOTICE: PHP message: PHP Deprecated:  Creation of dynamic property Drupal\Core\Logger\LoggerChannelFactory::$_serviceId is deprecated in /var/www/html/web/core/lib/Drupal/Component/DependencyInjection/Container.php on line 288"
[24-Jun-2022 15:55:56] WARNING: [pool app] child 218 said into stderr: "NOTICE: PHP message: PHP Deprecated:  Creation of dynamic property Drupal\Core\Session\AccountProxy::$_serviceId is deprecated in /var/www/html/web/core/lib/Drupal/Component/DependencyInjection/Container.php on line 288"
[24-Jun-2022 15:55:56] WARNING: [pool app] child 218 said into stderr: "NOTICE: PHP message: PHP Deprecated:  Creation of dynamic property Drupal\Core\Site\Settings::$_serviceId is deprecated in /var/www/html/web/core/lib/Drupal/Component/DependencyInjection/Container.php on line 288"
[24-Jun-2022 15:55:56] WARNING: [pool app] child 218 said into stderr: "NOTICE: PHP message: PHP Deprecated:  Creation of dynamic property Drupal\Core\Cache\CacheFactory::$_serviceId is deprecated in /var/www/html/web/core/lib/Drupal/Component/DependencyInjection/Container.php on line 288"
[24-Jun-2022 15:55:56] WARNING: [pool app] child 218 said into stderr: "NOTICE: PHP message: PHP Deprecated:  Creation of dynamic property Drupal\Core\Cache\ChainedFastBackendFactory::$_serviceId is deprecated in /var/www/html/web/core/lib/Drupal/Component/DependencyInjection/Container.php on line 288"
[24-Jun-2022 15:55:56] WARNING: [pool app] child 218 said into stderr: "NOTICE: PHP message: PHP Deprecated:  Creation of dynamic property Drupal\Core\Cache\DatabaseCacheTagsChecksum::$_serviceId is deprecated in /var/www/html/web/core/lib/Drupal/Component/DependencyInjection/Container.php on line 288"
[24-Jun-2022 15:55:56] WARNING: [pool app] child 218 said into stderr: "NOTICE: PHP message: PHP Deprecated:  Creation of dynamic property Drupal\Core\Cache\DatabaseBackendFactory::$_serviceId is deprecated in /var/www/html/web/core/lib/Drupal/Component/DependencyInjection/Container.php on line 288"
[24-Jun-2022 15:55:56] WARNING: [pool app] child 218 said into stderr: "NOTICE: PHP message: PHP Deprecated:  Creation of dynamic property Drupal\Core\Cache\ApcuBackendFactory::$_serviceId is deprecated in /var/www/html/web/core/lib/Drupal/Component/DependencyInjection/Container.php on line 288"
[24-Jun-2022 15:55:56] WARNING: [pool app] child 218 said into stderr: "NOTICE: PHP message: PHP Deprecated:  Creation of dynamic property Drupal\Core\Cache\ChainedFastBackend::$_serviceId is deprecated in /var/www/html/web/core/lib/Drupal/Component/DependencyInjection/Container.php on line 288"
[24-Jun-2022 15:55:56] WARNING: [pool app] child 218 said into stderr: "NOTICE: PHP message: PHP Deprecated:  Creation of dynamic property Drupal\Core\Config\CachedStorage::$_serviceId is deprecated in /var/www/html/web/core/lib/Drupal/Component/DependencyInjection/Container.php on line 288"
[24-Jun-2022 15:55:56] WARNING: [pool app] child 218 said into stderr: "NOTICE: PHP message: PHP Deprecated:  Creation of dynamic property Drupal\Core\Config\ExtensionInstallStorage::$_serviceId is deprecated in /var/www/html/web/core/lib/Drupal/Component/DependencyInjection/Container.php on line 288"
[24-Jun-2022 15:55:56] WARNING: [pool app] child 218 said into stderr: "NOTICE: PHP message: PHP Deprecated:  Creation of dynamic property Drupal\Core\Cache\ChainedFastBackend::$_serviceId is deprecated in /var/www/html/web/core/lib/Drupal/Component/DependencyInjection/Container.php on line 288"
[24-Jun-2022 15:55:56] WARNING: [pool app] child 218 said into stderr: "NOTICE: PHP message: PHP Deprecated:  Creation of dynamic property Drupal\Core\Cache\ChainedFastBackend::$_serviceId is deprecated in /var/www/html/web/core/lib/Drupal/Component/DependencyInjection/Container.php on line 288"
[24-Jun-2022 15:55:56] WARNING: [pool app] child 218 said into stderr: "NOTICE: PHP message: PHP Deprecated:  Creation of dynamic property Drupal\Core\Extension\ModuleHandler::$_serviceId is deprecated in /var/www/html/web/core/lib/Drupal/Component/DependencyInjection/Container.php on line 288"
[24-Jun-2022 15:55:56] WARNING: [pool app] child 218 said into stderr: "NOTICE: PHP message: PHP Deprecated:  Creation of dynamic property Drupal\Core\DependencyInjection\ClassResolver::$_serviceId is deprecated in /var/www/html/web/core/lib/Drupal/Component/DependencyInjection/Container.php on line 288"
[24-Jun-2022 15:55:56] WARNING: [pool app] child 218 said into stderr: "NOTICE: PHP message: PHP Deprecated:  Creation of dynamic property ArrayObject::$_serviceId is deprecated in /var/www/html/web/core/lib/Drupal/Component/DependencyInjection/Container.php on line 288"
[24-Jun-2022 15:55:56] WARNING: [pool app] child 218 said into stderr: "NOTICE: PHP message: PHP Deprecated:  Creation of dynamic property Drupal\Core\Validation\ConstraintManager::$_serviceId is deprecated in /var/www/html/web/core/lib/Drupal/Component/DependencyInjection/Container.php on line 288"
[24-Jun-2022 15:55:56] WARNING: [pool app] child 218 said into stderr: "NOTICE: PHP message: PHP Deprecated:  Creation of dynamic property Drupal\Core\Config\TypedConfigManager::$_serviceId is deprecated in /var/www/html/web/core/lib/Drupal/Component/DependencyInjection/Container.php on line 288"
[24-Jun-2022 15:55:56] WARNING: [pool app] child 218 said into stderr: "NOTICE: PHP message: PHP Deprecated:  Creation of dynamic property Drupal\Core\Config\ConfigFactory::$_serviceId is deprecated in /var/www/html/web/core/lib/Drupal/Component/DependencyInjection/Container.php on line 288"
172.30.1.3 -  24/Jun/2022:15:55:56 +0000 "POST /index.php" 200

1. +++ b/core/lib/Drupal/Component/DependencyInjection/Dumper/OptimizedPhpArrayDumper.php
   @@ -432,6 +432,7 @@ protected function dumpValue($value) {
   +      @trigger_error('_serviceId is deprecated in drupal:9.4.0 and is removed from drupal:10.0.0. See https://www.drupal.org/project/drupal/issues/2531564.', E_USER_DEPRECATED);
   
   +++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/DependencySerializationTraitPass.php
   @@ -8,6 +8,9 @@
   + * @deprecated in drupal:9.4.0 and is removed from drupal:10.0.0. The _serviceId property is no longer part of the container.
   + * @see https://www.drupal.org/project/drupal/issues/2531564
   

   updated to 9.5 and filed CR https://www.drupal.org/node/3292540

2. +++ b/core/modules/views/src/ViewExecutable.php
   @@ -2490,10 +2490,12 @@ public function getDependencies() {
   +    $container = \Drupal::getContainer();
   +    $serviceIdMapping = \Drupal::service('kernel')->getServiceIdMapping();
   ...
   -      'views_data' => $this->viewsData->_serviceId,
   -      'route_provider' => $this->routeProvider->_serviceId,
   +      'views_data' => $serviceIdMapping[$container->generateServiceIdHash($this->viewsData)],
   +      'route_provider' => $serviceIdMapping[$container->generateServiceIdHash($this->routeProvider)],
   

   changed to approach from #2987089: Remove views.module's direct use of the _serviceId property

Fixed deprecation message, meantime it means that \Drupal\Tests\Component\DrupalComponentTest is fragile

I wonder whether it would be possible to design the API in a way which leaks less implementation details. Looking through the patch, I think the most important call site is DependencySerializationTrait::__sleep(). The current patch wants to change the implementation into something like this (irrelevant parts removed):

      $container = \Drupal::getContainer();
      $mapping = \Drupal::service('kernel')->getServiceIdMapping();
      foreach ($vars as $key => $value) {
        [...]
            $id = $container->generateServiceIdHash($value);
            if (isset($mapping[$id])) {
              $service_id = $mapping[$id];
            }
          }
          if ($service_id) {
        [...]

I'd prefer something like this:

      $container = \Drupal::getContainer();
      foreach ($vars as $key => $value) {
        [...]
            $service_id = \Drupal::service('kernel')->getServiceId($value);
          }
          if ($service_id) {
        [...]

This will simplify changing the implementation in the future if the mapping/hashing approach has issues which we haven't found yet. It also makes it more obvious for contrib on how to migrate code which used the _serviceId property before.

Another approach would be:

      $container = \Drupal::getContainer();
      $lookup = \Drupal::service('kernel')->getReverseServiceLookup();
      foreach ($vars as $key => $value) {
        [...]
            $service_id = $lookup->getServiceId($value);
          }
          if ($service_id) {
        [...]

Like this it is still possible to update the lookup table in getReverseServiceLookup (i.e. outside the for loop).

IMO #174 looks better

Interesting measures of serialisation https://peakd.com/hive-168588/@crell/benchmarking-serialization

Are we fixing #2536370: DependencySerializationTraitPass does not add _serviceId to decorated services here? And can we add test coverage to ensure we do... FYI this will need a reroll now that #2987089: Remove views.module's direct use of the _serviceId property has landed.

Re-roll after #2987089: Remove views.module's direct use of the _serviceId property

If we need _serviceId on every service it looks reasonable to add #[\AllowDynamicProperties] to every service, but meantime this is blocked on phpstan (see #3275858-4: View's ResultRow uses deprecated dynamic properties for details of test run)

Patch for 10.0.x to test #[\AllowDynamicProperties]

Not clear why phpStan throws on php 8.2 - trying without \

  Line   core/lib/Drupal/Core/Extension/Extension.php             
 ------ --------------------------------------------------------- 
  13     Class AllowDynamicProperties is not an Attribute class.  

Filed https://github.com/JetBrains/phpstorm-stubs/pull/1404 to make phpstan find the attribute on PHP 8.2

Fix failed test - this test addresses #177 concern about testing decorated services