Problem/Motivation

Continuing the trend (1, 2, 3, 4) of adding more service locators in core. This issue resolves to convert user permission callbacks as defined in *.permissions.yml files to anonymous services, bundled together in a service locator, injected to a dedicated locator service, which is then used by PermissionHandler to build a list of available permissions.

The result, will be that devs can create permission providers in the same manner as existing, but with proper (tagged) services, not a bespoke definition in a YAML file. In turn, since these become real services, we'll be able to use Attributes and Autoconfig[er/u]ration.

The ultimate goal (out of scope) would be to simply create a file in a module, add an attribute to a method, without the need for a services.yml entry. However we are a couple of steps away from that. So here are the building blocks for permissions as tagged services.

Future

After this issue, we can look to implement a namespace + attribute so we dont need to manually define permission providers in either a services.yml/permissions.yml. It'll get discovered automatically, for example:

namespace Drupal\my_module\PermissionProvider;

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

#3422359: Directory based automatic service creation can be used to realize this feature.

Proposed resolution

  • Allow permission providers to be defined as tagged services.
  • Promote existing permission providers from permissions.yml files to services
  • Implement a service locator for the tagged services.
  • Utilise the service locator in PermissionHandler

Remaining tasks

Review, Merge.

User interface changes

None.

API changes

  • Existing permission providers are converted to anonymous services (unusable outside of the locator)
  • Arguments for user.permissions modified.
  • Deprecation message for extenders of \Drupal\user\PermissionHandler
  • New service locator service, accessible by autowiring.

Data model changes

Nil.

Release notes snippet

Permission providers may now be implemented as tagged services instead of callbacks in module.permissions.yml files.

Change record: https://www.drupal.org/node/3421580

CommentFileSizeAuthor
#17 3421573-nr-bot.txt90 bytesneeds-review-queue-bot
#13 3421573-nr-bot.txt90 bytesneeds-review-queue-bot

Issue fork drupal-3421573

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:

About issue forks

Comments

dpi created an issue. See original summary.

dpi opened merge request !6615

dpi’s picture

dpi
Location Perth, Australia
CreditAttribution: dpi at PreviousNext commented
Issue summary: View changes
Status: Active » Needs review

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) as the 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.

dpi’s picture

dpi
Location Perth, Australia
CreditAttribution: dpi at PreviousNext commented
Issue summary: View changes

Created a change record: https://www.drupal.org/project/drupal/issues/3421573

dpi’s picture

dpi
Location Perth, Australia
CreditAttribution: dpi at PreviousNext commented
Assigned: dpi » Unassigned

Updated MR with feedback.

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
CreditAttribution: kim.pepper at PreviousNext commented
Related issues: +#2339487: Static cache permissions

This issue might conflict #2339487: Static cache permissions

dpi’s picture

dpi
Location Perth, Australia
CreditAttribution: dpi at PreviousNext commented

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.

dpi’s picture

dpi
Location Perth, Australia
CreditAttribution: dpi at PreviousNext commented
Issue summary: View changes
Related issues: +#3422359: Directory based automatic service creation

#3422359: Directory based automatic service creation will fulfill the future possibilities for automatic permission provider registration.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: kristiaanvandeneynde at Factorial GmbH commented

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.

dpi’s picture

dpi
Location Perth, Australia
CreditAttribution: dpi at PreviousNext commented
Assigned: Unassigned » dpi
Status: Needs review » Needs work

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

dpi’s picture

dpi
Location Perth, Australia
CreditAttribution: dpi at PreviousNext commented
Status: Needs work » Needs review

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. Overall a win.

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

Tests back to green.

dpi’s picture

dpi
Location Perth, Australia
CreditAttribution: dpi at PreviousNext commented
Assigned: dpi » Unassigned
needs-review-queue-bot’s picture

needs-review-queue-bot CreditAttribution: needs-review-queue-bot as a volunteer commented
Status: Needs review » Needs work
StatusFileSize
new 3421573-nr-bot.txt90 bytes

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.

dpi’s picture

dpi
Location Perth, Australia
CreditAttribution: dpi at PreviousNext commented
Status: Needs work » Needs review

Fixed conflicts after #2926070: Deprecate ModuleHandlerInterface::getName().

Still needs review.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented

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.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented

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

needs-review-queue-bot’s picture

needs-review-queue-bot CreditAttribution: needs-review-queue-bot as a volunteer commented
Status: Needs review » Needs work
StatusFileSize
new 3421573-nr-bot.txt90 bytes

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.

usmanjutt84’s picture

usmanjutt84
he/him
Primary language Urdu
Location Islamabad
CreditAttribution: usmanjutt84 as a volunteer commented
Issue summary: View changes

Updated the "Change record" URL.

nicxvan’s picture

nicxvan CreditAttribution: nicxvan commented

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

usmanjutt84’s picture

usmanjutt84
he/him
Primary language Urdu
Location Islamabad
CreditAttribution: usmanjutt84 as a volunteer commented

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.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: kristiaanvandeneynde at Factorial GmbH commented

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.

joachim’s picture

joachim CreditAttribution: joachim as a volunteer commented

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.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: kristiaanvandeneynde at Factorial GmbH commented

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

joachim’s picture

joachim CreditAttribution: joachim as a volunteer commented

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.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: kristiaanvandeneynde at Factorial GmbH commented

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.