Email processors currently all inherit from a single interface which only defines the init() method.
Processors for specific phases need to define a method corresponding to the phase(s) they want to perform processing in.
While this is simple to do, it is also a bit hard for both humans and tools to discover these methods are related, and which arguments are required.
Define a new interface (inheriting from EmailProcessorInterface which clearly define the method needed to apply processing in that phase.
Make all existing processors implement the interfaces they need for the phases they operate in.
Change EmailProcessorBase and EmailProcessorCustomBase to look check if it implements the interface when registering processor callbacks.
Add some comments letting tools know where the interfaces are used. (@uses references).
None.
For now, the interfaces are made optional by leaving the current method_exists() calls so things don't break without warning.
We could add some deprecation warning to the release notes, or maybe even trigger a user deprecation message when a processor which does not implement the corresponding interface is discovered.
None.
Show commandsStart 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:
changes
Comments
Comment #3
twodComment #4
adamps commentedInteresting, thanks for the issue and patch.
Note that at the moment it's not mandatory to chose specific function names. The processor could override the init function and call the functions whatever they like - even dynamically based on parameters of the specific email. The functions could come from any other class even, or not from a class. Probably it's not common or recommended. It would be fairly easy to fix by adding a function of the canonical name that calls on to whatever function is required.
For me the patch is bit complex. When writing a processor we have to remember to implement the interface and define the function. If we remove a function, or switch to a different phase then we have to change the interfaces too. Inside this module the patch adds close to 200 lines of code and 4 interfaces. I feel that if we are going to solve this issue, let's make the new code really simple.
EmailProcessorBaseandEmailProcessorCustomBasecan define empty implementations of all 4 functions, then we can remove the if from the init() function. It's fully back-compatible with no deprecation. It means we add a bunch of empty processors onto each email but hopefully the performance impact will be negligible. In core for example Entities and Forms both have methods that are empty by default.What do you think?
Comment #5
twodThanks for the quick feedback.
Btw, are you by any chance at DrupalCon right now? Maybe we could meet up and have a chat between sessions? Trying to pay attention to a speaker while writing this so it may be a bit incoherent. :D
Yes, I thought of that approach and had the same concern about implementing an empty default callback for every processor & phase. So, I went with this approach as it had minimal impact on existing custom (3rd party) implementations. It took a long time to figure out what the latest API changes were about (all our custom processors broke) when the processors were suddenly not able to make the same email modifications in the same phases when upgrading to the latest alpha. Especially since the in-module processors are full of
@inheritdocreferences but they don't actually inherit from anything - and the only way to find where they were called from was breapoints or tedious searching. So, I figured there was at some point an intention to have interfaces and a more type safe way to deal with the phase methods - at least as the default way to do it.The whole point of adding the interface(s) was that you do need to do those things to be sure you have type safety. Changing phases is an API break. Changing what arguments a phase callback receives is an API break. Adding more phases would be an API addition (backwards compatible), but removing one would certainly break a lot.
Thinking about it a bit more I agree having the default methods there would probably not be that much of an impact. As you say, core already does it in several places.
If you inherit from a base class potentially inheriting from a single interface with all phases defined you don't have to worry about API additions since there should be new empty defaults. I agree having multiple interfaces is a bit more complex, but it would be slightly smoother if you don't inherit from the base class (which does seem like an unlikely case).
I know the module is still in alpha, but removing the existing registration loop in init() would break all custom processors that do not use the "default" method names - which was allowed - so there should at least be a clear entry about it in the changelogs.
Comment #6
twodDisregard most of what I said about backwards compatibility and processors not getting registered above, misunderstood what you were saying.
I slimmed down the changes like you suggested and merged the interfaces. By default we have processors based on the provided base classes, which use the interfaces, and we keep the ability to not use them at all.
Comment #7
adamps commentedGreat thanks I like it. No I'm not at DrupalCon - I'm semi-retired these days😃. I added some comments into the MR.
Comment #8
adamps commentedI've fixed those comments and pushed a new version.
@uses may be technically correct but seems very non-standard in the Drupal world, so I switched to the familiar @see.
Comment #9
adamps commented