[Symfony 5] Services are private by default

The first patch just upgrades to symfony/dependency-injection:5.2 to find the failures.

The second patch has various fixes extracted from #3161889: [META] Symfony 6 compatibility to get the tests running, let's see how this fares on the full set.

One more fix for some kernel tests.

In the first patch in #2, run-tests.sh can't even boot without a fatal error:

Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException: The "module_handler" service or alias has been removed or inlined when the container was compiled. You should either make it public, or stop using the container directly and use dependency injection instead. in /var/www/html/drupal/vendor/symfony/dependency-injection/Container.php:266

Fixed two more tests.

No idea what is up with InlineBlockPrivateFilesTest or InlineBlockTest.

No, it doesn't work at all - we get fatal errors before we even run any tests with the 5.2 dependency injection component, see the first patch in #2.

Figured out a fix for the inline block tests, however I'm not sure how we can notify contrib that they might need to add setPublic() calls to their service providers, or if we can do this in another way.

Symfony services have been private by default since 3.4, but Drupal still uses public by default - we need public services because we directly access the container in lots of places such as plugin factory methods.

There has been this sort of halfway house BC layer in Symfony where services could be declared both public and private and that still worked for us, but that layer has now been removed in Symfony 5.2 and this is what we are running into.

Re #13.3 the problem I had here is that setDefinition() could assume public by default, but then what if a caller really wants to add a private service? How would setDefinition tell the difference between a definition that should be public-by-omission vs explicitly set private? The Definition object belongs to Symfony so we can't add any extra flags to detect this.

This is the same reason ContainerBuilderTest has to change, I'm not sure this is right either but I don't see what the alternative is.

Addressed #13.1, #13.2 in this patch.

#13.3 is still outstanding as per #14 and I am not sure what to do.

#13.4 we not changed YamlFileLoaderTest here, which proves that private services remain private, shortened version:

    $yml = <<<YAML
  example_private_service:
    class: \Drupal\Core\ExampleClass
    public: false
YAML;
...
    $this->assertFalse($builder->getDefinition('example_private_service')->isPublic());
    $builder->compile();
    $this->assertFalse($builder->has('example_private_service'));

Wrong interdiff above, sorry - correct one is here.

Re #15 the setPrivate(false) used to work until SF 5.1 but now setPrivate() is fully deprecated in SF 5.2:

The code we had before in our setDefinition() implementation:

    if ($definition->isPublic()) {
      $definition->setPrivate(FALSE);
    }

This does nothing, because setPrivate() now just does this:

    public function setPrivate(bool $boolean)
    {
        trigger_deprecation('symfony/dependency-injection', '5.2', 'The "%s()" method is deprecated, use "setPublic()" instead.', __METHOD__);

        return $this->setPublic(!$boolean);
    }

ie. setPrivate(false) now just calls setPublic(true), which we already checked in the if statement.

Had a thought that I will try out in a minute: by default Definitions are both public and private by default in SF4, which is why we need that explicit setPrivate(FALSE). But we could detect this default and warn the caller via a deprecation that in Drupal 10 they will explictly need to call setPublic(TRUE) if they want a public service; if they make this change it would remove the deprecation as setPublic() does the setPrivate(FALSE) part for us.

So I tried the idea in #22 and added a deprecation that is triggered only when isPublic() and isPrivate() are both true, which as far as I can see only happens with definitions that haven't explicitly been declared either way in SF4 only.

The attached two patches are the same underlying set of changes, the SF4 patch stays on SF4 and includes a legacy test for the new deprecation, the SF5 patch upgrades to SF5 but doesn't include the legacy test as the new deprecation is never triggered (and in the D10 cycle we would remove this anyway).

There is now a failure in OptimizedPhpArrayDumperTest that I don't understand and also setDefinition() is still being called without public/private being set properly from somewhere, but leaving this for a bit and will look at it fresh another day unless someone else gets there first!

1) Drupal\Tests\Core\DependencyInjection\ContainerBuilderTest::testLegacySetDefinition
Failed asserting that false is true.

Why is this failing in the SF5 patch? Ideally I think both patches should be green here.

@daffie Thank you so much for working on this. This is looking great now, just one minor improvement:

+++ b/core/tests/Drupal/Tests/Core/DependencyInjection/Compiler/StackedKernelPassTest.php
@@ -38,13 +38,24 @@ protected function setUp(): void {
+    $kernel_basic = $this->createMiddlewareServiceDefinition(FALSE, 0);
+    $kernel_basic->setPublic(TRUE);
...
+    $kernel_three = $this->createMiddlewareServiceDefinition();
+    $kernel_three->setPublic(TRUE);
...
+    $kernel_one = $this->createMiddlewareServiceDefinition(TRUE, 10);
+    $kernel_one->setPublic(TRUE);
...
+    $kernel_two = $this->createMiddlewareServiceDefinition(TRUE, 5);
+    $kernel_two->setPublic(TRUE);

Here I think we could move ->setPublic(TRUE) to the helper method?

Thanks! Good news that this is green now on both Symfony 4 and 5. Sorry for not picking this up before, but I think as per #19/#20 we need to remove the deprecation skips from the Symfony 4 patch that is to be committed? Once that is done this is RTBC as far as I can see.

I think the SF5 patch will fail now due to the new deprecations, but assuming the SF4 patch is green that one is good to go.