The method \Drupal\eca\EventSubscriber\EcaBase::onEvent currently handles only the event object, that's being passed on by \Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher::dispatch. That dispatcher also passes on the event name and the context.
We should also handle the event name as the second argument, pass that on to the processor and from there to \Drupal\eca\Entity\EcaStorage::loadByEvent to only load those event, that really are used in models.
Ran into that issue while implementing eca_state_machine where they have one event class WorkflowTransitionEvent which is used with a huge variety of event names. But those event names are not included anywhere other than that second argument described above.
I'll post a fix, shouldn't be that difficult.
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 #2
jurgenhaasComment #4
jurgenhaasThis is working as expected and all tests are still passing.
Comment #5
mxh commented\Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher::dispatchstates regarding the$eventNameparameter:That means
NULLor nothing is allowed to be set, meaning it's not guaranteed to have an event name defined. That needs to be taken care of accordingly. This is relevant when using Symfony-based packages within Drupal that are not aware of Drupal.When having an event name, it could be considered using the event name instead of the event class in general. I currently don't see that it makes sense to use both within
EcaStorage::loadByEventand thus looks like unnecessary overhead.Comment #6
jurgenhaasThat's correct, calling
dispatchcan omit the event name, but the dispatch method itself already makes sure that the event name is being set with a string value before triggering the callable from the subscriber. So, there we can rely on the event name being present. Unless I'm missing something?That's a good point, the event name should be unique where the class isn't. I'll give that a try.
Comment #7
jurgenhaasYes, that works as well and the modified code is contained in the MR for another review.
Comment #8
mxh commentedThe problem is, that this is not a guarantee from that method, because it's neither documented nor contracted in any way. The only thing where this is "defined" is by its current inner implementation. But it may be very unlikely that Drupal is changing that behavior in the future, thus I don't see this as a blocker. We should generally rely on officially API contracts though and only fall back to "trusting" inner implementation logic.
Comment #10
jurgenhaasThanks for the clarification, you're absolutely right. In this case, at least we know for sure that all model tests would be failing right away, if that inner implementation in core would have changed.
Comment #11
mxh commentedNow that we take that event name into account, it might also make sense to drop "drupal_event_class" in favor of "drupal_id" everywhere it is currently used (especially on appliance checks). At least we should consider passing it along the
::appliesmethods as it is now part of the pre-emptive loading of configurations. We should also consider renaming "drupal_id" to "drupal_event_name" for sake of consistency. We could address this as a separate issue if you agree.Comment #12
jurgenhaasGreat idea, I'm all up for this. Removing the
drupal_event_classcompletely should be the goal. I've just scanned the code and it's almost possible:$this->drupalEventClass()in plugins can be replaced withdrupal_id$this->plugin->drupalEventClass()in\Drupal\eca\Entity\Objects\EcaEvent::appliescan be replaced with the event name as you suggested.\Drupal\eca\Event\TriggerEvent::dispatchFromPluginwhere we determine the event class from the ECA event plugin.Let's address this in a new issue though.
Comment #13
mxh commentedCreated follow-up