This is something I think should also happen in core with entity handlers, but I'll try it in Group 2.0 first.

Currently, the state of entity type or group plugin handlers is that they can only be swapped out once. It's a nightmare for two modules to try and alter the same handler of an entity type or plugin defined by another module. The goal is to alleviate this problem.

Defaults

Each handler tends to have a base class with sane defaults, this will from now on become a service so that anyone can decorate said service to their heart's content. The handlers themselves need to be written with decoration in mind:

  • Concise interfaces
  • Very limited scope

More info on how to decorate a service multiple times here: Symfony docs

Specific implementation

Now how do we allow modules to define their specific implementation of the base class? They should create a service and link to that in the annotation rather than the actual class. This means that anyone trying to target a specific entity type or handler's alterations, can do so by decorating the defined service.

Keep in mind we used to want to swap out many entity type's handlers in modules such as Group or Entity API, but now that we have a "defaults" service, we can do our heavy lifting there and not worry about having to alter the "specifics" extensions of said services.

Hooking up specifics to defaults

Now this is the hard part: We need to artificially inject the defaults service into the specific one so that it too can act as a chain of decorators around the defaults. The problem is that given a defaults chain of C > B > A and a specific chain of Z > Y > X, where X was the original handler defined by the module, we need to make sure that the final order becomes Z > Y > X > C > B > A.

By merely having a setter on the handler, this will end up with it being called on Z first with C, whereas it should automatically get Y injected by Symfony. So we need a way to make sure X and only X receives C as its "inner" property.

But here's the beauty of it: If we define X with the service that represents A as a dependency, we will automatically get C injected because C decorates B which in turn decorates A. Then, any definition of Y and Z will not get C injected directly.

The only downside is documenting this new behavior, because people are used to look at the default class and extend it using the inheritance pattern. Now we need to teach them to look at the default service name and write their own service that takes the default as an argument.

A concrete example

Let's say we have a "permission provider" handler (both Entity API and Group have this notion) and we want to ask for the available permissions for a given thing (entity type or group plugin). This is what that could look like:

# group.services.yaml
# Default service. Defines default CRUD permissions.
group.group_plugin.permission_provider:
  class: 'blah blah'
  arguments: ['@some.argument']

# g_content_translation.services.yaml
# Adds translation permissions for all plugins that support translation.
g_content_translation.group_plugin.permission_provider:
    decorates: group.group_plugin.permission_provider
    decoration_priority: 5
    arguments: ['@.inner']

# gnode.services.yaml
# Overrides some default permission labels
gnode.group_plugin.permission_provider:
    arguments: ['@group.group_plugin.permission_provider']

# secretnodes.services.yaml
# Overrides some things ONLY for nodes.
secretnodes.group_plugin.permission_provider:
    decorates: gnode.group_plugin.permission_provider
    decoration_priority: 5
    arguments: ['@.inner']

This means that any Media entity would inherit permissions defined by by group and g_content_translation, whereas Node entities would see permissions coming from all 4 services.

Comments

kristiaanvandeneynde created an issue. See original summary.

kyuubi’s picture

Great analysis @kristiaanvandeneynde,

I think you're right regarding the documentation side effects, but I personally don't see that as a deal breaker.

This approach looks very good to me and quite elegant as well!

kristiaanvandeneynde’s picture

jonathanshaw’s picture

It'd be lovely to forge a way to solve this problem in core. I'm wondering if the same strategy would also work for entity classes, which are tricky to enhance for the same reasons.

kristiaanvandeneynde’s picture

Status: Active » Needs review
StatusFileSize
new220.72 KB

A big-ass proof of concept that I expect to go red, but the tests take 50 minutes on my Docker and 5 on drupal.org, so here we are.

Status: Needs review » Needs work

The last submitted patch, 5: group-3203186-new-handlers-POC.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

kristiaanvandeneynde’s picture

Status: Needs work » Needs review
StatusFileSize
new221.3 KB

Fixed a few silly mistakes let's see what really fails now.

Status: Needs review » Needs work

The last submitted patch, 7: group-3203186-new-handlers-POC-2.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

kristiaanvandeneynde’s picture

Status: Needs work » Needs review
StatusFileSize
new221.6 KB

Hmm, bit worried it can't find the handler classes even thought the namespaces seem to be correct.

Status: Needs review » Needs work

The last submitted patch, 9: group-3203186-new-handlers-POC-3.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

kristiaanvandeneynde’s picture

Status: Needs work » Needs review
StatusFileSize
new220.84 KB

This should fix a few blatant mistakes.

Status: Needs review » Needs work

The last submitted patch, 11: group-3203186-new-handlers-POC-4.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

kristiaanvandeneynde’s picture

Status: Needs work » Needs review
StatusFileSize
new223.1 KB

This should fix a bunch of unit tests.

Status: Needs review » Needs work

The last submitted patch, 13: group-3203186-new-handlers-POC-5.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

kristiaanvandeneynde’s picture

Status: Needs work » Needs review
StatusFileSize
new223.14 KB

Most is fixed now. Query access is acting up because handlers were optional before 2.0.0 and now they are not. Will look into fixing those final few bits.

Status: Needs review » Needs work

The last submitted patch, 15: group-3203186-new-handlers-POC-6.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

kristiaanvandeneynde’s picture

Status: Needs work » Needs review
StatusFileSize
new225.93 KB

Found a few more things.

Status: Needs review » Needs work

The last submitted patch, 17: group-3203186-new-handlers-POC-7.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

kristiaanvandeneynde’s picture

Status: Needs work » Needs review
StatusFileSize
new5.83 KB

Yaml is picky about how you format booleans. TIL.

kristiaanvandeneynde’s picture

StatusFileSize
new227.68 KB

Wrong file, my bad

kristiaanvandeneynde’s picture

StatusFileSize
new225.58 KB

Forgot I need to diff vs dev, not 2.0.0

kristiaanvandeneynde’s picture

Status: Needs review » Fixed

Committed to 2.0.x. We now have 2.0.x-dev!

Will try to tag this issue as that version as soon as I have the option to.

kristiaanvandeneynde’s picture

Version: 8.x-1.x-dev » 2.0.x-dev
kristiaanvandeneynde’s picture

Handler services are now created for you: #3222970: Automatically create missing handler services.

Status: Fixed » Closed (fixed)

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