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.

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

Command icon 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:

Comments

longwave created an issue. See original summary.

longwave’s picture

Status: Active » Needs review
spokje’s picture

Besides 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?

longwave’s picture

Status: Needs review » Needs work
Issue tags: +Needs change record

Yeah, 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.

longwave’s picture

Status: Needs work » Needs review
Issue tags: -Needs change record

As 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.

spokje’s picture

Status: Needs review » Reviewed & tested by the community

I think we can add BC and point the deprecation notice directly to this issue.

Never 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:

This service is private and I think the chances of anyone overriding it are low.

I agree chances are pretty low on that.

longwave’s picture

Re #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.

spokje’s picture

I'm OK with that :)

  • catch committed e85a9e85 on 11.x
    Issue #3416353 by longwave, Spokje: Convert CheckProvider to use a...
catch’s picture

Status: Reviewed & tested by the community » Fixed

I 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!

Status: Fixed » Closed (fixed)

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