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
Comment | File | Size | Author |
---|---|---|---|
#25 | 2939208-25.patch | 4.61 KB | alexpott |
#25 | 18-25-interdiff.txt | 1.05 KB | alexpott |
#18 | 2939208-18.patch | 4.41 KB | alexpott |
#18 | 2939208-18.test-only.patch | 1.82 KB | alexpott |
#14 | 2939208-14.patch | 2.59 KB | alexpott |
Comments
Comment #2
tim.plunkettComment #3
alexpottI think this means we can remove the
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.Comment #4
pounardI 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.
Comment #5
alexpottFor 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.
Comment #6
tim.plunkettFrom \Drupal\simpletest\TestServiceProvider:
This wrongly causes deprecations for all calls to
'router.route_provider'
in kernel tests.The patch above fixes that.
Comment #7
alexpottI 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.Comment #8
larowlanThis is at least major, possibly critical, as ideally we'd fix it before 8.5.0
Comment #9
BerdirYes, 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.
Comment #10
BerdirAw, firefox and it's stupid remembering of form values even when you reload the page :)
Comment #11
BerdirBecause it's not just a deprecation message as the issue title says, it actually breaks the module, in case that wasn't clear :)
Comment #12
alexpottI 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.
Comment #13
alexpottFound 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 setSYMFONY_DEPRECATIONS_HELPER
toweak_vendors
. It's interesting that this breaks a module because the deprecation is not supposed to cause the service to go missing.Comment #14
alexpottHere's what I was thinking in #3.
Comment #15
brunodboI was directed to this issue by @larowlan, since I was getting the error
You have requested a non-existent service "access_check.csrf"
ondrush 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.Comment #16
larowlancan we get a test-only patch for this?
what else is there to do here
Comment #17
alexpott@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.
Comment #18
alexpottHere's a test for the changes to ContainerBuilder::setDefinition(). The test-only patch is the interdiff.
Comment #20
alexpottI think this is pretty weird but not sure we can do anything about it.
Comment #21
alexpottThe 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.
Comment #22
alexpottSo this leads us to another fix for the paragraphs problem.
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.Comment #23
alexpottI'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.
Comment #24
alexpottUpdated issue summary to be clearer about why this is critical.
Comment #25
alexpottI've tried to update the comment in the patch and the issue title to reflect the my recent comments.
Comment #26
dawehnerThis 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?
Comment #27
Berdir@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?
Comment #28
dawehnerWell, 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.
Comment #29
BerdirAnything 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.
Comment #31
larowlanCommitted as 3dad711 and pushed to 8.6.x
Cherry-picked as de17645 and pushed to 8.5.x
Agree