[PP-1] Drupal ContainerBuilder allows mutation of non-synthetic services

Ah ha. I was digging around patches and the meta issues but either missed that issue somehow or missed alex bringing it up in the reviews. I'll make a patch and see the impact. If its pretty trivial we can maybe toss it back over there.

So took a lot of back and forth yesterday with chx on slack to step through some seemingly unrelated changes required for this. Going to post a patch to kick off some tests then explain in a review.

1. +++ b/core/lib/Drupal/Core/DependencyInjection/ContainerBuilder.php
   @@ -62,21 +63,21 @@ protected function shareService(Definition $definition, $service, $id, array &$i
   +    // Force services to be defined with lowercase identifiers. Originally
   +    // required to harden against case-insensitivity of service identifiers.
        if (strtolower($id) !== $id) {
          throw new \InvalidArgumentException("Service ID names must be lowercase: $id");
        }
   

   Updating the doc block made it clear this was never documented. Went ahead and included some docs but it looks to be cruft that could even get in the way and should probably be removed. If we want to continue to have a standard we should use static analysis to enforce it.

2. +++ b/core/lib/Drupal/Core/DependencyInjection/ContainerBuilder.php
   @@ -62,21 +63,21 @@ protected function shareService(Definition $definition, $service, $id, array &$i
   +    try {
   +      parent::set($id, $service);
   +    }
   +    catch (BadMethodCallException $e) {
   +      @trigger_error($e->getMessage() . ' Doing so is deprecated in drupal:10.0.0 and the method is removed in drupal:11.0.0. Make service static or refactor service usage. See https://www.drupal.org/node/3300306', E_USER_DEPRECATED);
   +      SymfonyContainer::set($id, $service);
   +    }
   

   I could have copied the checks from symfony but this seemed maybe the best way to avoid keeping in sync with any bug fix or improvements happening up stream which is the intention. If we did this in 9 for 10 the maintenance wouldn't matter as much and copying the logic would probably be better. I don't know why I said static, I meant synthetic.

3. +++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/BackendCompilerPass.php
   @@ -49,7 +49,6 @@ public function process(ContainerBuilder $container) {
            $driver_backend = $container->get('database')->driver();
            $default_backend = $container->get('database')->databaseType();
   -        $container->set('database', NULL);
   

   This is required by cause Symfony's container builder removes the definition if you unset the service. This is a feature of it being a _builder_ so this hack doesn't work anymore. Why was this hack here and why don't we need it anymore? Stick around for a tale...

   First the reason for it existing. Without this line, the entire _serviceId sleep magic falls apparent and we suddenly start serializing database connections.

   You might be asking how could that possibly be? This has nothing to do with serialization right?

   Well our first clue is just above this change we can see the code ask the container to create the database connection but to see why we have to look at another change.

4. +++ b/core/lib/Drupal/Core/CoreServiceProvider.php
   +++ b/core/lib/Drupal/Core/CoreServiceProvider.php
   @@ -58,6 +58,7 @@ public function register(ContainerBuilder $container) {
   
   @@ -58,6 +58,7 @@ public function register(ContainerBuilder $container) {
        // service definitions. This pass must come first so that later
        // list-building passes are operating on the post-alter services list.
        $container->addCompilerPass(new ModifyServiceDefinitionsPass());
   +    $container->addCompilerPass(new DependencySerializationTraitPass());
    
        $container->addCompilerPass(new ProxyServicesPass());
    
   @@ -93,7 +94,6 @@ public function register(ContainerBuilder $container) {
   
   @@ -93,7 +94,6 @@ public function register(ContainerBuilder $container) {
        // Register plugin managers.
        $container->addCompilerPass(new PluginManagerPass());
    
   -    $container->addCompilerPass(new DependencySerializationTraitPass());
   

   Now is where this weird change comes in. See that database driver creation happens in a compile pass before the "DependencySerializationTraitPass" has run. That means our service container doesn't have the magic that tricks Symfony into writing _serviceId onto the object. Which mean our connection doesn't have one and the house of cards collapses.

   This particular change should probably not be committed as is because we have a better fix of removing the pass entirely in #2531564: Fix leaky and brittle container serialization solution but for things to generally work for testing this is needed.

Oh yeah, I forgot about that. um... there's an issue to remove the persist tag on request services. Attached

So, I think the right way to handle this might be postpone this on the better fixes for some of the underlying issues and working with the BC they add.