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

This issue might conflict #2339487: Static cache permissions

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 #3422359: Directory based automatic service creation 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.

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.

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.

I am also still weak -1 on having to specify the method, it feels like permissions services should just have a fixed interface; most modules will have 0 or 1 of these services, so I don't see why they all have to allow individual method names to be configured. And if we had a fixed interface we could autoconfigure it so the tag doesn't even need to be specified in services.yml.

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.

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

Experimented a bit using an attribute to define a permission provider (see TaxonomyPermissions.php).
It works, seems easy to use, ....

I didn't remove the option to use permission_callbacks in the MR yet but it's easy to deprecate that here.

Settings to "Needs review" to get some thoughts about MR 15644.

Does the service need to be in a particular sub-namespace?

No, see https://git.drupalcode.org/project/drupal/-/merge_requests/15644/diffs#f.... All classes using the attribute are tagged with user.permission_provider without the need of a specific sub-namespace.

Please also consider my proposal to add sections and extended help to permissions files:

https://www.drupal.org/project/drupal/issues/3495351

E.g., there could be:

namespace Drupal\my_module\PermissionProvider;

class MyPermissionProvider {
	
	#[AsPermissionProvider]
	public function permissions(): array {}

	#[AsPermissionSectionProvider]
	public function sections() array {}
}

The nomenclature would need to be worked on.

I think that just as EventSubscribers go in a standard namespace, we should do the same here.

It's an opportunity to establish convention before we need to worry about BC.

Having things in standard locations improves DX.

I still think we should introduce an interface contract here, what is the use case for having a permission provider service with two tagged methods? Seems better to have a fixed method name (as we do elsewhere), we don't even need the attribute, we can use autoconfiguration to tag and collect PermissionProviderInterface (or whatever name we choose) services...

> what is the use case for having a permission provider service with two tagged methods

That's not what the MR does. The MR adds the attribute to the class.

I meant that TestPermissionProvider has two methods, __invoke() and getPermissions() , both of which can provide permissions, what's the use case here? Why do we need attributes at all?

Ah sorry.
Using the attribute without any parameter will call __invoke(). By settings the parameter you are able to change that to a method.
The test simply shows both ways.

Implemented #34 in MR!15845 - no attribute needed, just a new PermissionProviderInterface interface with a single method. Services that implement this method are collected into a service iterator which is injected into PermissionHandler.

I prefer @longwave's approach in MR 15845. The IS mentions attributes a future state about autodiscovery, but that would be a future state, and I think using autoconfiguration only for now makes sense.

Until we can do general autodiscovery via #3422359: Directory based automatic service creation, I think we should be judicious about adding autodiscovery for specific things, because there is always the problem of reflecting classes with missing optional dependencies.

Had a few comments on the MR.

Is there a consideration for when these are collected and compiled?

@tolstoydotcom, I think that's a great idea, but likely a follow up issue.

The collection point hasn't changed, I think moving that here is out of scope.

Addressed MR feedback, back to NR.

Can we restrict these to a subfolder for consistency and DX.

Do you mean restricting the attribute version? My suggestion just uses services, we shouldn't restrict those.

I mean making permission providers go in a specific subfolder, for better DX.

We do that already with EventSubscriber don't we? And hooks are services and they go in a specific place.

It's not strictly enforced, but I guess we could - but if we move the class do we also need to provide BC? Not sure there is a use case for calling permission providers separately, or extending them, but maybe someone's doing it somewhere?

Having said that given that each module is likely to have zero or one permission provider services, what's really wrong with Drupal\module\ModulePermissions?

Added a suggestion to the MR that hopefully covers all the deprecation test failures.

It's not strictly enforced, but I guess we could - but if we move the class do we also need to provide BC? Not sure there is a use case for calling permission providers separately, or extending them, but maybe someone's doing it somewhere?

Calling permission providers separately also seems unlikely to me, but there may be a use case of extending a permission provider if someone wanted to replace the provider with their own class that extends the core class. It occurs to me that with the class being changed to a service, a subclass could break anyway if it is expecting to implement ContainerInjectionInterface and ::create(). Maybe the service should be a new class, and the old class deprecated as a whole?

How would someone override a permission provider before though? There is no mechanism to alter the YAML inside PermissionHandler::buildPermissionsYaml(). In theory you could extend an existing provider and register it separately, but in practice would anyone actually have done that?

Yeah, you're right. There's no way to alter the definitions from YAML, and at a glance of existing providers, there's very little there for a module to extend in order to register its own provider.

I also think Drupal\module\ModulePermissions is fine as convention.

> I also think Drupal\module\ModulePermissions is fine as convention.

I'd be okay with that.

We should then revive #2910814: Deprecate magic ServiceProvider file discovery, as we could make that follow the same pattern be Drupal\module\ServiceProvider.