[Symfony 6] Update EventDispatcherInterface type hints in constructors

+1. Similar approach at #3157895: Move 'install_time' state update from \Drupal\Core\Installer\Form\SiteConfigureForm to installed_finished() which is what I was referring to in our Slack thread about this.

Even if the arguments aren't type hinted, the PHPDoc can say:

@param $whatever FooInterface|BarInterface

So long as the code comment to explain what's happening uses my "prevent cascade of WTF" formulation, I'm happy. 😉 The code would be approximately this:

    // Prevent a cascade of WTF if a caller passes in something totally wrong, since we're not type hinting this.
    if (!$event_dispatcher instanceof FooInterface && !$event_dispatcher instanceof BarInterface) {
      throw new \InvalidArgumentException('The fourth argument must implement either \Symfony\Component\...\EventDispatcherInterface or \Symfony\Contracts\...\EventDispatcherInterface.');
    }

More or less...

Cheers,
-Derek

Please avoid assigning issues to your self as this is discourage for D.O contributors in general.
Kindly go though the given link for better understanding of do's and don't

https://www.drupal.org/docs/develop/issues/issue-procedures-and-etiquett...

@Meenakshi.g: assigning issues to yourself is fine if you are going to work on them in a short timeframe. See the linked docs.

Reviewing the current patch I don't have *strong* feelings whether this manual checking is better or not than swapping out the interface in Drupal 10. I am leaning towards just doing the replacements in Drupal 10 otherwise we need to mark these code paths somehow for removal in Drupal 10, at least they would not be applicable then (unless we want to keep a BC layer in Drupal 10, which I don't think we want).

Found these minor code style things:

+++ b/core/lib/Drupal/Core/Field/FieldStorageDefinitionListener.php
@@ -25,7 +26,8 @@ class FieldStorageDefinitionListener implements FieldStorageDefinitionListenerIn
-   * @var \Symfony\Component\EventDispatcher\EventDispatcherInterface
+   * @var
+   * \Symfony\Component\EventDispatcher\EventDispatcherInterface|\Symfony\Contracts\EventDispatcher\EventDispatcherInterface

+++ b/core/lib/Drupal/Core/Form/FormBuilder.php
@@ -40,7 +41,8 @@ class FormBuilder implements FormBuilderInterface, FormValidatorInterface, FormS
-   * @var \Symfony\Component\EventDispatcher\EventDispatcherInterface
+   * @var
+   * \Symfony\Component\EventDispatcher\EventDispatcherInterface|\Symfony\Contracts\EventDispatcher\EventDispatcherInterface

+++ b/core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php
@@ -48,7 +49,8 @@ class HtmlRenderer implements MainContentRendererInterface {
-   * @var \Symfony\Component\EventDispatcher\EventDispatcherInterface
+   * @var
+   * \Symfony\Component\EventDispatcher\EventDispatcherInterface|\Symfony\Contracts\EventDispatcher\EventDispatcherInterface

The line break is not supposed to be here, is it? It is not in the other half of changes in the file.

Hi,

Creating a patch as per the #9 comment suggestion.

Please review the patch.

Let me know if any suggestions.

Thanks.

+++ b/core/lib/Drupal/Core/Action/Plugin/Action/GotoAction.php
@@ -28,7 +29,8 @@ class GotoAction extends ConfigurableActionBase implements ContainerFactoryPlugi
+   * @var \Symfony\Component\EventDispatcher\EventDispatcherInterface|\Symfony\Contracts\
+   * EventDispatcher\EventDispatcherInterface

I think the line breaks here and elsewhere are wrong too, at least PhpStorm doesn't understand the class name when it is broken over two lines.

Updated patch as per the #11 comment suggestion.

The line breaks in @var are still not right. We are allowed to go over 80 characters here, it must be kept on a single line:

+++ b/core/lib/Drupal/Core/Action/Plugin/Action/GotoAction.php
@@ -28,7 +29,8 @@ class GotoAction extends ConfigurableActionBase implements ContainerFactoryPlugi
+   * @var \Symfony\Component\EventDispatcher\EventDispatcherInterface|
+   * \Symfony\Contracts\EventDispatcher\EventDispatcherInterface

should be

+   * @var \Symfony\Component\EventDispatcher\EventDispatcherInterface|\Symfony\Contracts\EventDispatcher\EventDispatcherInterface

I also wonder if instead of doing this dance we can add some kind of BC/FC layer with class_alias()?

Symfony\Component\EventDispatcher\EventDispatcherInterface looks like this:

namespace Symfony\Component\EventDispatcher;

use Symfony\Contracts\EventDispatcher\EventDispatcherInterface as ContractsEventDispatcherInterface;

interface EventDispatcherInterface extends ContractsEventDispatcherInterface

ie. the new interface is the parent of the old interface. So can't we just widen the type in all these constructors now? Any caller that still passes a Component EventDispatcherInterface will satisfy Contracts EventDispatcherInterface.

As an example:

+++ b/core/lib/Drupal/Core/Action/Plugin/Action/GotoAction.php
@@ -48,12 +49,15 @@ class GotoAction extends ConfigurableActionBase implements ContainerFactoryPlugi
+  public function __construct(array $configuration, $plugin_id, $plugin_definition, $dispatcher, UnroutedUrlAssemblerInterface $url_assembler) {
+    if (!$event_dispatcher instanceof EventDispatcherInterface && !$event_dispatcher instanceof ContractsEventDispatcherInterface) {
+      throw new \InvalidArgumentException('$event_dispatcher must be an instance of ' . ContractsEventDispatcherInterface::class . ' or ' . EventDispatcherInterface::class);
+    }

The first half of the check is redundant because EventDispatcherInterface is an instance of ContractsEventDispatcherInterface already. And so this can be reduced to:

+    if (!$event_dispatcher instanceof ContractsEventDispatcherInterface) {
+      throw new \InvalidArgumentException('$event_dispatcher must be an instance of ' . ContractsEventDispatcherInterface::class . ' or ' . EventDispatcherInterface::class);
+    }

But we don't even need this check - we can just use the parameter type?

Straight find and replace as per #16.

All instances of Symfony\Component\EventDispatcher\EventDispatcherInterface have been replaced by Symfony\Contracts\EventDispatcher\EventDispatcherInterface. I did a code base search.
All code changes in the patch look good to me.
For me it is RTBC.

Should we also replace Symfony\Component\EventDispatcher\Event with Symfony\Contracts\EventDispatcher\Event? As the first is also deprecated since Symfony 4.3. See: https://github.com/symfony/event-dispatcher/blob/4.4/Event.php

@catch: Thank you for your explanation!

I think this has broken Drush, https://github.com/drush-ops/drush/issues/4614

Looks like forgot to unassign.

Thank you for the roll-back in #27 - yes this killed 75 of the tests in the Rules module at core 9.2. For information, the error was:

TypeError: Argument 1 passed to Drupal\rules\Logger\RulesLog::__construct() must be an instance of Symfony\Component\EventDispatcher\EventDispatcherInterface, instance of Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher given, called in /home/travis/build/jonathan1055/drupal/core/lib/Drupal/Component/DependencyInjection/Container.php

Those daily tests failed only on that one day, after the commit in #23. After the revert, they all run OK again. Is there any type of "simulated" contrib testing modules that are run as part of d.o. testing? It would seem that this fault should have been detectable?

To fix this issue we need to keep the existing class: Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher which implements Symfony\Component\EventDispatcher\EventDispatcherInterface and the class: Drupal\Component\EventDispatcher\Event which extends the class Symfony\Component\EventDispatcher\Event.
The problem is that the class Symfony\Component\EventDispatcher\Event is deprecated and that we need to replace the interface Symfony\Component\EventDispatcher\EventDispatcherInterface with the interface Symfony\Contracts\EventDispatcher\EventDispatcherInterface. As we cannot create a BC break, we must keep the existing classes.

Creating replacements for the two classes can be named in a number of ways:

1. Rename the classes. Something like: Drupal\Component\EventDispatcher\ContractsContainerAwareEventDispatcher and Drupal\Component\EventDispatcher\ContractsEvent;
2. Rename the component: Drupal\Component\ContractsEventDispatcher\ContainerAwareEventDispatcher and Drupal\Component\ContractsEventDispatcher\Event;
3. Rename the directory: Drupal\Contracts\EventDispatcher\ContainerAwareEventDispatcher and Drupal\Contracts\EventDispatcher\Event.

My question which should we choose?

@catch: Thank you for working on this issue. When I look at your patch it only replaces Symfony\Component\EventDispatcher\EventDispatcherInterface with Symfony\Contracts\EventDispatcher\EventDispatcherInterface. Unfortunately there needs to be done more in this issue. The class Drupal\Component\EventDispatcher\Event extends the class Symfony\Component\EventDispatcher\Event and that class is also deprecated. See: https://github.com/symfony/event-dispatcher/blob/4.4/Event.php.

@daffie That class was added to provide a forward-compatibility layer, downstream users of the Symfony Event class should now extend the Drupal class and then in Drupal 10 we can seamlessly switch the parent class from Symfony\Component to Symfony\Contracts.

See #3055198: [Symfony 5] Symfony/Component/EventDispatcher/Event is deprecated in Symfony 4.3 use Symfony/Contracts/EventDispatcher/Event instead for more info.

Rerolled #35.

Thanks for updating the patch.

1) Went through all changes and confirmed Component is changed to Contracts.

2) When looking for remaining instances of Symfony\Component\EventDispatcher\EventDispatcherInterface, it's referenced below which isn't part of the issue summary:

./tests/Drupal/Tests/Component/EventDispatcher/ContainerAwareEventDispatcherTest.php:    $this->expectDeprecation('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');
./lib/Drupal/Component/EventDispatcher/ContainerAwareEventDispatcher.php:use Symfony\Component\EventDispatcher\EventDispatcherInterface;
./lib/Drupal/Component/EventDispatcher/ContainerAwareEventDispatcher.php:      @trigger_error('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', E_USER_DEPRECATED);

3) Patch applies to 9.2.

4) Tests pass.

5) One thing in the issue summary and noted in comment #2:

@dww suggested checking the two interfaces in __construct() and throwing InvalidArgumentException if neither is passed.

This isn't covered in the patch so either it should be removed from the issue summary or added to the patch or a follow-up issue should be created.

6) Another thing in the issue summary:

We could eventually have an issue to add the Contracts type hint back, but it wouldn't be tied to a major release development window.

Does this issue need to be created as a follow-up issue so it's not forgotten?

Thanks for updating the issue summary and providing clarification. Looks good to me based on last 2 comments so marking RTBC.

Committed c4d6699 and pushed to 9.2.x. Thanks!