Access policies should be marked as final to encourage decorating

I dont feel too strongly about this. The OP makes some good arguments for this. Some counter-arguments:

• Its a bit unrealistic to have multiple access policies magically work together. Each site would have different requirements about what happens when the policies overlap
• Is decorating a class really much safer than extending it. You still "depend" on the inner class in a significant way.
• I'm from the "we're all consenting adults" philosophy. I would slap @internal on the policy classes and then any developer who chooses to extend does so at own risk.

Replying to #3:

Each site would have different requirements about what happens when the policies overlap

Emphasis mine. I am mainly worried about contrib. If two modules want to drastically alter another module's or core's access policy in a way that they can't do from their own policy's calculatePermissions() or alterPermissions() then they would both end up extending the contrib/core policy, making them incompatible with the other module.

If they both were to decorate said access policy, then both decorators would fire and you might get lucky. But you'll never get lucky if we allow extending/swapping over decorating.

Is decorating a class really much safer than extending it.

It's not about safety or BC, that's merely what got me thinking. It's about interoperability, something which is completely impossible when using extensions. If multiple modules want to slightly affect an existing third-party policy, they can without breaking the others.

I would slap @internal on the policy classes and then any developer who chooses to extend does so at own risk.

But that's the thing, while they are largely internal and you should be able to get the job done with a custom policy's alterPermissions() there might be valid edge cases where you still want to alter the original class. If we mark it as @internal, that might leads to people being hesitant about decorating it, even though we definitely want to encourage decorating.

TL;DR: I would like to see an ecosystem where interoperability isn't necessarily guaranteed, but it's highly likely. By allowing extensions, core is shooting itself in the foot because as soon as one module chooses to extend over decorating, the gate is immediately closed for all other modules to do the same.

Can the MR be updated to point to 11.x or a 2nd one opened up? SInce both 10.3 and 11.0 are in beta.

Done

as soon as one module chooses to extend over decorating, the gate is immediately closed for all other modules to do the same.

If custom or contrib code makes a new policy and chooses to extend another (the original policy could be active or inactive - both valid use cases), why would all other active policies cease to work as designed? I must be missing something.

Okay, let's say you have a module that extends UserRolesAccessPolicy to the following:

1. Change persistent cache contexts to one representing a sitewide flag
2. In calculatePermissions(), if said flag is ON, return the parent and make sure we add user.roles context. If OFF, do nothing.
3. In your service override file, set access_policy.user_roles class to your extending class

Now we have another module that wants to do something else to UserRolesAccessPolicy and also decides to extend said class. Now how would that module work together with module A? They are both trying to replace alter container's definition of access_policy.user_roles to point to their class. Only one can win.

With decorators, the same results would be achievable, but both modules would be able to make their modification to UserRolesAccessPolicy.

In my example I said "makes a new policy".

Your example is generally about the drawbacks of altering services from other modules. We have already settled that concern with permitted, but not recommended (see italicized warning) → . final takes this warning a lot further. Your example is a perfectly reasonable thing for a site to do, and the final makes it harder. Site developers don't need to care about multiple cooperating access policies.

Many cooperating policies is a rare use case. We've learned this with node access grants system - we built a system which handles multiple cooperating policies but few understand the system. We would benefit from a simpler and less cooperative system here IMO. Sorry I'm going off topic here.

IMO, this comes down to how paternal we want our code to be. I slightly prefer less paternal.

Ah right, making a new policy based on an existing one was also talked about on Slack: If you based your policy off of an existing one and leave the existing one intact, then you're not using the API properly.

The original policy will provide a list of permissions and your policy will provide a highly likely very similar list because it used the original policy as its starting point. But all permissions are merged together at the end of the loop over all policies. So basing your access policy off of anything but the base class is a waste of computing resources because you're calculating something that's already been calculated.

Same goes for the alterPermissions() call, it's intended to add or remove items after the calculate phase. If you were to extend an existing policy, you'd be running the same code twice even though this code only has to run once to be effective.

Keep in mind a site can still swap out access policy services even if they're final. Simply decorate said service and never call the parent where you don't want to.

IMO, this comes down to how paternal we want our code to be. I slightly prefer less paternal.

Generally, me too. We have over 100 final classes in core and I don't necessarily agree with all of them. But this is where it really makes sense. Access policies should never overlap with one another, so there is absolutely no reason to extend one as a base of your own. Given how all methods are public by design, it's really built for decorating should the need ever arise.

Having said that, with both a build and alter phase, you should rarely run into a scenario where you need to actually fiddle with someone else's policies.

Can we please commit this, though? As @catch has pointed out on Slack, we can always remove final whenever we want (even in a minor or patch release) and it would not constitute a BC break. Adding it, however, would require a new major release and a deprecation process.

I consider the majority of the Access Policy API as part of the user subsystem of which I'm the maintainer along with Moshe. I'll gladly pledge here that if anyone at any point comes up with a real-life practical example that simply cannot be solved in any other way than to extend an existing policy, I'll gladly remove the final keyword for them. I'm willing to commit to that.

They can point me to this very comment and make me eat it.

So can we pretty please get this in before 10.3 is released?

If @catch said it can be removed later, don't mind marking.

Yeah I am skeptical about final everywhere but where we really don't think something should be an extension point (because we know there are other, better, ones), it makes sense to use it. Note that 🌱 Use final to define classes that are NOT extension points Active is open to discuss where and how to use it.

Not sure why there were 11.x and 10.3.x MRs here since they were identical, just cherry-picked the 11.x commit. Committed/pushed to 11.x and cherry-picked back to 10.3.x, thanks!

catch → closed merge request !8228

catch → closed merge request !8214

Thanks! I'll make sure to update the documentation to reflect these changes come Monday.

Added a note to the BC policy → and the documentation →

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