Problem/Motivation

Drupal uses \Drupal\Component\DependencyInjection\Dumper\OptimizedPhpArrayDumper to dump the container to a PHP array (which is then cached). The dumper only dumps private services if they are referenced from a public service. In Symfony 3.4, all services are marked private and public by default (this is BC layer to Symfony 4). There is a Symfony container compiler pass \Symfony\Component\DependencyInjection\Compiler\ResolvePrivatesPass that will make any service that has not has setPublic(TRUE) called on it to private. The combined affect of OptimizedPhpArrayDumper, the new defaults in Definition and ResolvePrivatesPass means that services that used to be in the container are no longer. This is affect contrib and core patches - see #2927746-93: Update Symfony components to 3.4.* and #2938355: Missing revision_uid column

Proposed resolution

Change the ContainerBuilder::setDefinition() to make services not private if they are still public.

Remaining tasks

User interface changes

None

API changes

None

Data model changes

None

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett created an issue. See original summary.

tim.plunkett’s picture

Status: Active » Needs review
Parent issue: » #2927746: Update Symfony components to 3.4.*
FileSize
846 bytes
alexpott’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

I think this means we can remove the

    // As of Symfony 3.4 all services are private by default.
    $definition->setPublic(TRUE);

from \Drupal\Core\DependencyInjection\ContainerBuilder::register() now since $this->setDefinition() is called by the parent implementation.

There is also the same code in \Drupal\Core\DependencyInjection\YamlFileLoader. That might be able to be removed too.

pounard’s picture

I think the right solution would be to really set explicitely the visibility on services, and set to false as much services as possible. I don't know how Drupal would behave, but performance-wise, with Symfony, it's much better to have as much private services as possible. In an ideal world, no service would be public, and container->get would be banned.

alexpott’s picture

For Drupal 8 @pounard that is totally impossible to do in backwardsly compatible manner that won’t require massive changes to nearly every Drupal module. For Drupal 9 yes we should move to private services if we can. This issue is about a BC break introduced in Drupal 8.5 through Symfony 3.4 adoption.

tim.plunkett’s picture

From \Drupal\simpletest\TestServiceProvider:

      $container->setDefinition($id, new Definition('Drupal\simpletest\\' . $class));

This wrongly causes deprecations for all calls to 'router.route_provider' in kernel tests.
The patch above fixes that.

alexpott’s picture

I see. It's interesting that we're not seeing these failures in HEAD. Is that because we don't ever try to access router.route_provider via the container in Kernel tests. That seems unlikely though. I wonder why we're not seeing this failure.

larowlan’s picture

Priority: Normal » Major

This is at least major, possibly critical, as ideally we'd fix it before 8.5.0

Berdir’s picture

Priority: Major » Normal

Yes, this is breaking paragraphs.module tests for example because we dynamically register an replicate event subscriber if that module is installed. See #2938355: Missing revision_uid column.

I'd vote to make this critical as I can imagine a lot of contrib modules being affected by this. There is an easy workaround but it would still break a ton of existing sites if they update and don't also update all those modules, which would also need to do new releases.

Berdir’s picture

Priority: Normal » Major

Aw, firefox and it's stupid remembering of form values even when you reload the page :)

Berdir’s picture

Because it's not just a deprecation message as the issue title says, it actually breaks the module, in case that wasn't clear :)

alexpott’s picture

Priority: Major » Critical

I think what @Berdir says makes this critical. We have this in core too - see \Drupal\rest\RestServiceProvider::register() all those services will now be private so any module that depends on rest and used \Drupal::service() to get them will be broken.

alexpott’s picture

Found out why we're not seeing this in HEAD. It's because the deprecation is triggered from \Symfony\Component\DependencyInjection\ContainerBuilder::get() and this is a vendor deprecation. Vendor deprecations are ignored if you use run-tests.sh or have updated your phpunit.xml file to set SYMFONY_DEPRECATIONS_HELPER to weak_vendors. It's interesting that this breaks a module because the deprecation is not supposed to cause the service to go missing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
1.71 KB
2.59 KB

Here's what I was thinking in #3.

brunodbo’s picture

I was directed to this issue by @larowlan, since I was getting the error You have requested a non-existent service "access_check.csrf" on drush cache-rebuild (8.5-alpha1). The patch in #14 fixes it for me.

Looks like the error in my case might have been because of Flag module modifying the access_check.csrf service.

larowlan’s picture

+++ b/core/tests/Drupal/KernelTests/KernelTestBaseTest.php
@@ -137,6 +137,10 @@ public function testRegister() {
+    // Ensure getting the router.route_provider does not trigger a deprecation
+    // message that errors.
+    $this->container->get('router.route_provider');

can we get a test-only patch for this?

what else is there to do here

alexpott’s picture

@larowlan it doesn't fail! That would only fail if SYMFONY_DEPRECATIONS_HELPER was set to strict. I think we need to update the release notes and the related change record (https://www.drupal.org/node/2928884 or https://www.drupal.org/node/2936045) to detail that people's custom phpunit.xml files need to be updated for the changes we've made in phpunit.xml.dist.

alexpott’s picture

Here's a test for the changes to ContainerBuilder::setDefinition(). The test-only patch is the interdiff.

The last submitted patch, 18: 2939208-18.test-only.patch, failed testing. View results

alexpott’s picture

+++ b/core/tests/Drupal/Tests/Core/DependencyInjection/ContainerBuilderTest.php
@@ -70,6 +71,34 @@ public function testRegister() {
+    // Test a service with private set to true. Drupal does not support this.
+    // We only support using setPublic() to make things not available outside
+    // the container.
+    $definition = new Definition();
+    $definition->setPrivate(TRUE);
+    $service = $container->setDefinition('foo', $definition);
+    $this->assertTrue($service->isPublic());
+    $this->assertFalse($service->isPrivate());

I think this is pretty weird but not sure we can do anything about it.

alexpott’s picture

The reason the break is happening in Paragraphs is that \Symfony\Component\DependencyInjection\Compiler\ResolvePrivatesPass() is making things private and then our \Drupal\Component\DependencyInjection\Dumper\OptimizedPhpArrayDumper::getServiceDefinitions() is not dumping private definitions unless they are referenced. Event listeners are not referenced anywhere so the 'replicate.event_subscriber.paragraphs' service doesn't make it into our dumped container.

alexpott’s picture

So this leads us to another fix for the paragraphs problem.

diff --git a/core/lib/Drupal/Component/DependencyInjection/Dumper/OptimizedPhpArrayDumper.php b/core/lib/Drupal/Component/DependencyInjection/Dumper/OptimizedPhpArrayDumper.php
index dfd2a431cc..c63e84444b 100644
--- a/core/lib/Drupal/Component/DependencyInjection/Dumper/OptimizedPhpArrayDumper.php
+++ b/core/lib/Drupal/Component/DependencyInjection/Dumper/OptimizedPhpArrayDumper.php
@@ -128,6 +128,14 @@ protected function getServiceDefinitions() {
       }
     }
 
+    // Ensure all services are there.
+    foreach ($this->container->getDefinitions() as $id => $definition) {
+      if (!isset($services[$id])) {
+        $service_definition = $this->getServiceDefinition($definition);
+        $services[$id] = $this->serialize ? serialize($service_definition) : $service_definition;
+      }
+    }
+
     return $services;
   }
 

I used this to run 'Drupal\paragraphs\Tests\Experimental\ParagraphsExperimentalReplicateEnableTest' without the fix to make the definition public and it works. It might be the better and more correct fix.

alexpott’s picture

I've tried #22 and removing the public setting from our YamlFileLoader and ContainerBuilder. It does not work because of other assumptions made in our OptimizedPhpArrayDumper - it ends up in an infinite loop. Therefore I think #18 probably offers the best BC-compatibility. It would be nice to use Symfony's BC layer but it just does not appear simple.

alexpott’s picture

Issue summary: View changes

Updated issue summary to be clearer about why this is critical.

alexpott’s picture

Title: Unintentional deprecations because manually registered definitions are now private by default in Symfony 3.4 » Services registered with ContainerBuilder::setDefinition() are removed from container causing a BC break
FileSize
1.05 KB
4.61 KB

I've tried to update the comment in the patch and the issue title to reflect the my recent comments.

dawehner’s picture

This looks like a perfect valid fix. Thank you for expanding the testcoverage!

One question I have: Did you experimented with adding this logic to a compiler pass instead? Maybe it would be better to not add this custom logic into the builder, but execute it a bit later. Do you have thoughts about that?

Berdir’s picture

@dawehner: What would be the benefit of that? We have to do it on all services and in my profiling, compiler passes always show up as very expensive, every one that is added has to loop over all the services again, seems more efficient to just do it when setting the definition?

dawehner’s picture

Well, you know, it would be less hardcoded behaviour, but I think the required behaviour here is so fundamental that it makes sense to put it into the middle of it.

Berdir’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

Anything else to do here? I understand #28 as @dawehner being OK with how this is done, and we have tests. Would be good to have this fixed for the beta.

  • larowlan committed 3dad711 on 8.6.x
    Issue #2939208 by alexpott, tim.plunkett: Services registered with...
larowlan’s picture

Version: 8.6.x-dev » 8.5.x-dev
Status: Reviewed & tested by the community » Fixed

Committed as 3dad711 and pushed to 8.6.x

Cherry-picked as de17645 and pushed to 8.5.x

Would be good to have this fixed for the beta.

Agree

  • larowlan committed de17645 on 8.5.x
    Issue #2939208 by alexpott, tim.plunkett: Services registered with...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.