Problem/Motivation
At core 9.1 we get the following deprecation message:
Calling the Symfony\Component\EventDispatcher\EventDispatcherInterface::dispatch() method with a string event name as the first argument is deprecated in drupal:9.1.0, an Event object will be required instead in drupal:10.0.0. See https://www.drupal.org/node/3154407
Change Records
https://www.drupal.org/node/3154407 - Signature of EventDispatcherInterface::dispatch() has changed
https://www.drupal.org/node/3159012 - Symfony Event class deprecated, EventDispatcher::dispatch() argument order changed
| Comment | File | Size | Author |
|---|---|---|---|
| #16 | 3172039-16-dispatcher.patch | 5.6 KB | tr |
Issue fork rules-3172039
Show commands
Start 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:
Comments
Comment #2
jonathan1055 commentedThere are currently 516 warnings at core 9.1. I have fixed this in Scheduler module see #19 in #3166688-19: Deprecated: Calling EventDispatcherInterface::dispatch() with a string event name
Scheduler uses Rules in its tests as we provide Rules actions and conditions.
Just creating this issue to record the current state - I'm OK with having the allowed deprecation count for now, as we have other things to do in Rules which are more urgent.
Comment #3
tr commentedCore isn't going to backport this change to D8 so fixing this deprecation will have to wait until we branch Rules and have a separate D9 branch. We still have a few years until D10, which is when this change will be required ...
Comment #4
jonathan1055 commentedActually, we can fix this right now and its not too compex to do. This patch resolves all 516 of the warnings, and in essence all it does is intercept the places that dispatch an event, instead of dispatching directly, to use a function which swaps the parameter order when running on Core 9.1 and above before calling the real dispatch function. In summary, and numbering for ease of reference:
RulesLogclass in /src/Logger/RulesLog.php to re-use existing code, as this already has the \Symfony\Component\EventDispatcher\EventDispatcherInterface$dispatcherproperty, but I understand that this might not be the proper place for our dispatcher function. However, this was done for expediency and to demonstrate the point with minimal code change.or in test files the equivalent
$this->arguments = ['account' => $account];in the __construct() functiondispatch()function and also uses it in its own call for SystemLoggerEventtestInitEvent()andtestTerminateEventneeded to have an $event object to pass to the new dispatch() function, so this was simply added by$event = new GenericEvent()I am not saying we have to commit this, or even that this is the best solution, but wanted to demonstrate how it can be done, and to test it with druapalci.yml to show that the depecations can be fixed.
Comment #5
jonathan1055 commentedAs expected the patch failed due to the other deprecations, but there are none for the dispatch() parameter order.
See Travis test for 9.1 deprecation info https://travis-ci.org/github/jonathan1055/rules/builds/730185319
Only 347 deprecations in total (297 void hint and 50 placeholder prefix)
Compare with 9.1 test without this patch https://www.drupal.org/pift-ci-job/1832280
Comment #6
tr commentedNo, I don't want to do this. Rules needs to be able to react to any Symfony event, not just events generated through a Rules-specific mechanism. Plus this adds a mandatory Rules dependency to every contributed module that wants to provide Rules integration - right now those modules can just dispatch events whether or not Rules is enabled, and provide Rules plugins for conditions and actions which are just ignored if Rules is not installed. That wouldn't be true any more, and all those contributed modules would need to be changed.
This deprecation can be handled with a bunch of version_compare() calls, but we're going to have more and more of these non-BC changes in D9.1 as time goes on and each will require its own version_compare() - it's not going to be just this one. I really think this needs to be addressed in a separate minor branch after we switch to semantic versioning.- $event_dispatcher = \Drupal::service('event_dispatcher');
+ $event_dispatcher = \Drupal::service('logger.ruleslog');
Comment #7
tr commentedRe-opening now that D8 is no longer supported.
We should be able to do this by following the instructions in the change record.
Comment #8
beatrizrodriguesI'll work on that.
Comment #10
beatrizrodriguesSo, I changed almost every EventDispatcherInterface::dispatch() calls, but there is this one that I was not able to solve. There was this test called testTerminateEvent() and the call was like this:
A $event was missing, I was able to solve this problem at the testInitEvent() but I couldn't be able to do it in that testTerminateEvent(). I tried to create an event just like I did in the testInitEvent():
creating an event, but when I try to create an TerminateEvent, it requests me a response parameter, and I was not able to get it.
So, I would like some suggestions if it is possible. Thank you!!
Comment #11
beatrizrodriguesComment #12
tr commentedYour MR looks very good. I don't have an answer to your question right now, but I will look into it to see if I can find out what to do there.
Comment #13
tr commentedI think in the case of EventIntegrationTest, for both testInitEvent() and testTerminateEvent(), you can just create a new Drupal\Component\EventDispatcher\Event and dispatch it as
->dispatch($event, KernelEvents::REQUEST)or->dispatch($event, KernelEvents::TERMINATE). There is no context declared for these events in rules.rules.events.yml, so the only thing that really matters to Rules for these is the event name, since that's what we listen for and trigger on.This is a Kernel test, and the test removes all other listeners for these events because we are only trying to verify that Rules responds to them correctly. So for test purposes this should work just fine.
Comment #14
beatrizrodriguesthank you @TR I followed your suggestion. And also did a rebase. Thanks again
Comment #15
jonathan1055 commentedChanged parent to new meta issue #3257972: [meta] Rules deprecated code D9 -> D10
Comment #16
tr commentedThe MR needs to be rebased again - I'm not sure why, and I'm not sure why the auto rebase couldn't handle it. But also there are two unused use statements now, so I decided to just post a patch to update the MR.
@jonathan1055, do you want to add on the needed changes to .travis.yml for this fix? Drupal 9.1 is no longer supported, so we can fix D9.2 deprecations now, and it's nice to be aware of the new deprecations in D9.3 and D9.4.
Comment #18
tr commentedCommitted #16.
Comment #19
jonathan1055 commentedI did not get chance to check the deprecations on Travis before now. The commit has introduced a new one
We can leave this issue as fixed. I have raised a new issue
#3259445: Symfony\Component\EventDispatcher\Event is deprecated in drupal:9.1.0 to deal with this.
Comment #20
tr commentedYes, I'm aware of that, and have been running a fix for that locally for the past month or so. I have been doing some work with the event system and this is a little tricky to do in a BC way. I will post a patch in your new issue.
Comment #21
tr commentedComment #22
tr commented