Convert permission providers to tagged services; unify with a service locator

Implementation (MR) notes

The implementation here follows one I am familiar in with Messenger; classes are converted to anonymous tagged services, and grouped together with SendersLocator, which is then injected elsewhere.

The mapping between a method and a service is stored in a mapping array, and passed to the locator service so it knows which method to invoke on the service in locator. This mapping strategy is traditionally the way of storing in the locator which method is associated with a method tagged with a PHP attribute.

Services are created using the anonymous ID with hash + random strategy.

YAML discovery in PermissionHandler is still used for statically defined permissions. permission_callbacks is now completely ignored.

PermissionHandler has been switched to use autowiring. I think we can modify the services.yml definition at will, while the class constructor itself is able to handle deprecations (?)

Extensive use of PHPStan array shapes to improve static analysis of arrays.

Each way of implementing a permission provider, has been added to the new user_permission_provider_test.module.

The existing way of working exists, including compatibility with ControllerResolver/ContainerInjectionInterface.

If there is an existing service with the same ID (via autowiring) that class, it will be used instead of creating a new service.

The PermissionHandler unit test (PermissionHandlerTest) has a few YAML mocks removed, in favor of the new Locator with mock data. There is still a YAML test case there for necessary coverage. This unit test file has also been cleaned up a bit with redundant property assigns removed.

The permission provider locator (PermissionProvidersLocator) intentionally does not implement an interface, and the service is private. I don't see a need for it, and it could be added (and un-privatized) at a later date.

Created a change record: https://www.drupal.org/project/drupal/issues/3421573 📌 Convert permission providers to tagged services; unify with a service locator Needs review

Updated MR with feedback.

This issue might conflict 📌 Static cache permissions Needs work

Happy to deal with #2339487 whether it or this issue gets in before. Fortunately theres no dependency/blocker between these issues.

I've just added a review to that issues MR @ !6635.

✨ Directory based automatic service creation Needs review will fulfill the future possibilities for automatic permission provider registration.

Okay so after skimming this:

1. I like the idea
2. I like the use of modern php stuff, but am hesitant to see functions yield arrays with multiple pieces of information.
3. I'm not sure why you would use tagged services and then still allow people to specify callbacks in their permissions.yml files. To me it feels like we should use one or the other, not both. I'm personally in favor of tagged services.
4. For the time being I would allow and process both ways for BC reasons but throw a deprecation warning when callbacks are still detected in permissions.yml files.
5. Disclaimer: I haven't checked ✨ Directory based automatic service creation Needs review nor the 4 issues linked in the IS yet and maybe that might change some of the above points.

All in all I really appreciate you working on this as I fully agree what we currently have isn't modern at all. Listing callbacks inside yaml files feels a bit outdated.

Thanks for the feedback, I'll tweak the locator a little and deprecate callbacks in YAML.

I've refactored the permission locator slightly, per Kristiaan's feedback.

I've also deprecated permission_callbacks in YAML as suggested by Kristiaan. This change increases the LOC's significantly, but removes quite a lot of redundant lines in permission classes.

Changelog has been updated with deprecation wording, plus notes on deprecation scope — https://www.drupal.org/node/3421580 →

Tests back to green.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Fixed conflicts after 📌 Deprecate ModuleHandlerInterface::getName() Fixed .

Still needs review.

Why do we need a service locator here? We always need all tagged services, we don't need to refer to them individually for any reason, we just loop over the whole set - the service collector pattern seems to fit better? We could just add each permissions provider directly to the PermissionHandler.

Also, we shouldn't need to specify the "provider" tag manually, that should somehow be automatically inferred.

Is there a way to populate provider at discovery level instead of duplicating the module name?

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Updated the "Change record" URL.

I unpublished the change record, pretty sure it's not ready.

I agree. Thank you @nicxvan.

I appreciate the idea of generating dynamic permissions using services and tags, rather than relying on permission callbacks. The current implementation in the GraphQL 4 module utilizes the service name as a permission callback, which leads to errors on installin the module via drush because the service is not be registered at the time permissions are being generated. By adopting a service-based approach, this issue would be resolved in future releases, ensuring smoother functionality and enhanced reliability. See the issue service class does not exist on installing 🐛 service class does not exist on installing Active .

I just read @longwave's comment in #15 and I fully agree we don't need a locator here. A simple service collector (tagged services) would be better here. Repeating what he said:

• When we need the permission providers, we need all of them. So no point in having the option to lazy load them.
• When we want to build a permission UI or verify permissions on save, only then will we need the collector, so not using a locator has no drawbacks here

Furthermore I agree with the following:

I am also still weak -1 on having to specify the method, it feels like permissions services should just have a fixed interface

If a module for some reason needs multiple ways to provide permissions, it should simply provide multiple services; not multiple methods on a single service. Heavy +1 for a simple interface.

I think this is a change in the wrong direction. Declaring dynamic service provider classes in the same location as the fixed services is good DX.

This is a pattern we should be extending -- #2910814: deprecate magic ServiceProvider file discovery; declare in services.yml → -- not removing.

@joachim Are you alluding to the IS (which might need updating)? My last comment basically steers the MR towards tagged services, which are declared in the services.yml file. And that seems to be exactly what you want to see more of, or am I misreading this?

Argh I mistyped, sorry. I meant to say:

Declaring dynamic permissions provider classes in the same location as the fixed permissions is good DX.

So in other words, I think a dynamic service provider class should be declared in services.yml, and a dynamic permissions provider class should be remain as being declared in permissions.yml.

If you're looking for services or permissions, those files are where you go to look.

Normally I would agree, but as the IS states we're specifically looking to turning these providers into services so that we can benefit from everything the container has to offer us. If we were to keep them inside the permissions.yml file, we are severely limited as to what we can do.

One could even argue the permissions.yml file is another "magic file in a magic place" like the MyModuleServiceProvider class. In my opinion, the more callable code we can put into the container, the less custom code we have to write for these types of discoveries. The current controller approach means we need to inject the container to fetch said controller's dependencies, an approach that is not encouraged by Symfony (see removal of ContainerAwareTrait).

The way I see it:

• Permissions can only come from tagged services
• Core ships with one generic such tagged service that scours every module folder for a .permissions.yml file and gathers those
• Each module can then add as many such tagged services as they like, although usually one should suffice

Upside of this approach is that you can now turn off a service you don't want permissions from or decorate it to alter the result. With the current approach that's simply not possible.