Enable event plugins to determine appliance

This might be a potential blocker for 📌 Make all token data providers discoverable and expose them in the UI and in the ECA Guide Active because this change will introduce a general mechanic for event plugins being automatically included for conditional appliance and for being added as data provider.

Think I found a possible solution how to achieve the appliance behavior in the Processor service: The plugin manager for ECA events may provide a method for getting the according plugin ID of a system event. Then the processor can call a static method of the plugin's class, i.e. without the need to instantiate a plugin object on that early step. Created an MR390 that includes the approach.

In that MR I already refactored the eca_access submodule's events so that their logic of being a data provider and for conditional appliance got completely moved into the according plugin implementation. I didn't already refactor all other sub-modules, as this currently exceeds my time capacities.

For the intermediate phase, where it may be possible that either the system event or the plugin event implements being a data provider and/or conditional appliance, I decided to support both scenarios in this MR. It might make sense to support both scenarios even until 3.0.x, because it's a huge effort to refactor all existing implementations. If we'd want to enforce changing everything, then we'd also need to enforce other integrations and for that we'd need to remove the according interfaces entirely or move them into the plugin namespace.

When refactoring: For very different logics of data providers and conditional appliances, it might make sense for a plugin implementation to use separate child event plugin classes for it, instead of bloating up one plugin class with different cases. For such a situation, I think it's possible that you can manually define a 'class' entry within each entry of an implementation of Drupal\eca\Plugin\ECA\Event\EventInterface::definitions where you can point to a specific FQCN of a child class. I was already considering such a way in the eca_access refactoring example, but I think for that module it's still fine so I left it in one plugin class.

As always, this is outstanding! All the event handling in ECA is coming along really nicely, and it's really worth the refactoring as not only future maintenance will become much less difficult, but also integration of new events will be much more straightforward.

Think I found a possible solution how to achieve the appliance behavior in the Processor service: The plugin manager for ECA events may provide a method for getting the according plugin ID of a system event. Then the processor can call a static method of the plugin's class, i.e. without the need to instantiate a plugin object on that early step. Created an MR390 that includes the approach.

Yes, I like that approach.

In that MR I already refactored the eca_access submodule's events so that their logic of being a data provider and for conditional appliance got completely moved into the according plugin implementation. I didn't already refactor all other sub-modules, as this currently exceeds my time capacities.

Looking good, shall I alter the other sub-modules then?

For the intermediate phase, where it may be possible that either the system event or the plugin event implements being a data provider and/or conditional appliance, I decided to support both scenarios in this MR. It might make sense to support both scenarios even until 3.0.x, because it's a huge effort to refactor all existing implementations. If we'd want to enforce changing everything, then we'd also need to enforce other integrations and for that we'd need to remove the according interfaces entirely or move them into the plugin namespace.

ECA 2.0 will bring even more breaking changes for the event plugins, both for system event and ECA event plugins. So, all the integrations will have to be adjusted already, so I'd prefer to remove the double support now. TBH, what most integrations provide, are conditions and actions, but not events that much. That said, I think the overall amount of extra work seems reasonable.

When refactoring: For very different logics of data providers and conditional appliances, it might make sense for a plugin implementation to use separate child event plugin classes for it, instead of bloating up one plugin class with different cases. For such a situation, I think it's possible that you can manually define a 'class' entry within each entry of an implementation of Drupal\eca\Plugin\ECA\Event\EventInterface::definitions where you can point to a specific FQCN of a child class. I was already considering such a way in the eca_access refactoring example, but I think for that module it's still fine so I left it in one plugin class.

Yes, absolutely! Having a single plugin class combined with a deriver was an approach that allowed the EventSubscriber to get the static definition array without having to consult the event plugin manager. That's no longer needed, and therefore, implementing individual plugins will now be the preferred mechanic, I think. However, that can be done later as well, as it should not break anything and foremost doesn't introduce any API change.

I've committed a small code style change to the MR and also spotted a naming convention, that we haven't thought about before: I only noticed the machine-name token in the \Drupal\eca\Plugin\ECA\Event\EventBase::buildEventData method, which is really nice. However, we should consider using underscores instead of hyphens in token names. We've already updated that for current_user, current_form and others in the past, but we have more of those, that we should probably change. I know, these names got introduced before, I only noticed it now in the exposed location. And that made me realize that even more of those inconsistencies are around. But we should address this in a separate issue.

Thanks for looking into this, glad you like that approach as well. I added a comment into the Gitlab issue fork regards your commit, which would be great if that could be resolved.

Looking good, shall I alter the other sub-modules then?

If you want to take this part, yes sure.

So, all the integrations will have to be adjusted already, so I'd prefer to remove the double support now.

That's ok for me granted we've got all our sub-modules covered. I marked those double-supporting code blocks with a @todo comment in the MR and they can be removed then.

However, that can be done later as well, as it should not break anything and foremost doesn't introduce any API change.

Agreed.

also spotted a naming convention...

Thanks for creating a separate issue for that topic.

I have committed in 2 steps: the first commit moves all the ConditionalApplianceInterface implementations and the second one moves the DataProviderInterface implementations and removes the support of system events as data providers. Hope it is easier to review that way.

As for DataProviderInterface we do have a few implementations that are not ECA event related, e.g. \Drupal\eca_form\Token\CurrentFormDataProvider, \Drupal\eca_queue\Task, EcaExecutionEventDataSubscriber and a few in the Drupal\eca\Token namespace. I left them as they are, but WRT extracting provided tokens into the ECA Guide, we may have to find a way to discover all data providers.

Also, some of the ECA events have to override getData and since the interface forces a return type of ?DataTransferObject, I had to wrap all the returned data accordingly. Tests are still all green, but I'm not sure if that change may have some implication elsewhere?

I also commented on your thread in the MR, happy to further discuss that one.

since the interface forces a return type of ?DataTransferObject

The interface method Drupal\eca\Token\DataProviderInterface::getData is declared to return mixed, because it is allowed to return basically anything, not only DTOs.

we do have a few implementations that are not ECA event related

We need to evaluate, whether some of them can be moved into the plugins. If not, we could tag any service being a data provider (via services.yml), so that we can collect them into another service for our required discovery.

As of the currently too limiting (and thus wrong) return type of Drupal\eca\Plugin\ECA\Event\EventBase::getData I set this to NW and any adjustment that was made because of that return type limitation needs to be reverted because it doesn't make sense.

I have now reverted the return types in getData overrides and also updated the declaration in the base class. Also, resolved the thread and setting now back to NR.

That's great! Took a quick look and found following missing to do's:

• Since we want to drop double support, we need to entirely remove the interface Drupal\eca\Event\ConditionalApplianceInterface. For being able to do so, the only missing event that needs to be refactored is Drupal\eca_test_array\Event\ArrayWriteEvent.
• We then still have some leftovers to do regards data providers, such as various event subscribers like Drupal\eca\EventSubscriber\EcaExecutionAccountSubscriber, Drupal\eca\EventSubscriber\EcaExecutionEntitySubscriber, Drupal\eca\EventSubscriber\EcaExecutionFormSubscriber and some others where it would be better to have their logic covered by the according plugins as well. If we'd face different plugin implementations doing the same thing, maybe they could at least share a trait. I'm not sure whether that's to be addressed in 📌 Make all token data providers discoverable and expose them in the UI and in the ECA Guide Active anyway, feel free to ignore this point if so.

We probably have some data providers where it's not possible to provide a 100% clear definition / declarative layer, because their contents may vary by different context situations and user-defined configurations. The two where that's the case are Drupal\eca_queue\Task and Drupal\eca\Token\ContextDataProvider. But that is fine, we don't need 100% coverage or enforce all data providers being able to predict what kind of data they will provide. It's just important that we provide the mechanic for all data providers that can predict their types of provided data.

Setting to NW for the (probably last) required to do, which is the removal of Drupal\eca\Event\ConditionalApplianceInterface.

As mentioned in #15 we have some event subscribers, that make use of interfaces. For example Drupal\eca\EventSubscriber\EcaExecutionEntitySubscriber checks for Drupal\eca\Event\EntityEventInterface. I think for being more consistent in this refactoring manner, it would also make more sense to move that logic into the according plugins instead, and then finally removing that used interface (in this example Drupal\eca\Event\EntityEventInterface). IMO this is not a must-have, especially because that type of interface is also being used elsewhere in other implementations. Similar goes for other such interfaces like Drupal\eca\Event\FormEventInterface and Drupal\eca\Event\RenderEventInterface. Just mentioning here for sake of completeness.

I've addressed the last remaining todo and also adjusted the event template for the code generator. So, this seems ready to go as I agree with #15 and #16 to address the left over DataProvider tasks in #3348422 and will update that issue accordingly.

Great, nice work! Took a look into the MR and haven't found anyhing wrong. I couldn't check every changed imlementation in detail, as this is a huge change. But for that let's hope our existing tests are sufficiently covering.

Added a small commit that removes the event stack from the execution event subscriber, as it's not needed anymore.

Added a small commit that removes the event stack from the execution event subscriber, as it's not needed anymore.

That's great, thanks.

Merge this biggy, so we can now move on to the other steps. Feels great, this refactoring is even better than anticipated.

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