Problem/Motivation
In #3397522: Fork Symfony's ContainerAwareTrait and ContainerAwareInterface into core we are trying to reduce the use of ContainerAwareTrait as Symfony has deprecated it.
Drupal\Core\Access\CheckProvider is container aware because it needs to retrieve access checker services, but does not necessarily iterate over all of them.
Instead of injecting the container we can inject a service locator that only contains access checker services.
Steps to reproduce
Proposed resolution
Inject a service locator.
Also inject the dynamic_access_check_services parameter instead of retrieving it from the container.
Merge request link
Remaining tasks
Consider removing the dynamic_access_check_services parameter in a followup, as we can detect dynamic classes directly inside the service instead of injecting the list separately.
User interface changes
API changes
Data model changes
Release notes snippet
Issue fork drupal-3416353
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
- 3416353-convert-checkprovider-to
changes, plain diff MR !6279
Comments
Comment #3
longwaveComment #4
longwaveComment #5
spokjeBesides a question in the MR about this needing a CR this seems fine to me.
If it _does_ need a CR we might consider doing removing the dynamic_access_check_services parameter in here to prevent a CR on another CR?
Comment #6
longwaveYeah, let's write a change record just in case. Probably need this for the sister issues too...
Unsure about refactoring dynamic_access_check_services here as well, that would probably warrant a separate change record anyway as we are removing a container parameter.
Comment #7
longwaveAs in the LoggerChannelFactory issue I think we can add BC and point the deprecation notice directly to this issue.
This service is private and I think the chances of anyone overriding it are low.
Comment #8
spokjeNever seen that before, but hey, you're a Core Committer, so that's probably OK.
The only reason I can think of that it wouldn't be is that phpstan-drupal and drupal rector watch the published CRs automagically to determine if action is needed in either.
But with this:
I agree chances are pretty low on that.
Comment #9
longwaveRe #8 over in #3112298: Replace REQUEST_TIME in classes with direct container access we linked the new $time argument messages directly to the issue, so I assume it's OK to do here as well.
Comment #10
spokjeI'm OK with that :)
Comment #12
catchI also think linking to the issue directly is a reasonable approach where we don't expect anyone to have to make changes and the CR would would have nothing useful to say about the change. Committed/pushed to 11.x, thanks!