Problem/Motivation

With the release of Symfony 4.3 the event dispatching changed: https://symfony.com/blog/new-in-symfony-4-3-simpler-event-dispatching
This already causes issues for drupal regarding the upgrade to Symfony 4.x or 5, for example in
#3055194: [Symfony 5] The signature of the "Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher::dispatch()" method should be updated to "dispatch($event, string $eventName = null)", not doing so is deprecated since Symfony 4.3. and #3055180: [META] Symfony 5 compatibility.

But Drupal isn't the only system that has to deal with that. Third party libraries drupal leverages directly or via contributed modules need to adapt to this change. Therefore the symfony project itself provides a migration path by decorating the event dispatcher with LegacyEventDispatcherProxy as described in blog post linked above.

This LegacyEventDispatcherProxy has been released as part of symfony/event-dispatcher 4.3.0.
The event-dispatcher has no dependency to any 4.3 versions of other symfony components but is backward compatible to symfony 3.4 which we use in Drupal 8:

"require-dev": {
        "symfony/dependency-injection": "~3.4|~4.0",
        "symfony/expression-language": "~3.4|~4.0",
        "symfony/config": "~3.4|~4.0",
        "symfony/http-foundation": "^3.4|^4.0",
        "symfony/service-contracts": "^1.1",
        "symfony/stopwatch": "~3.4|~4.0",
        "psr/log": "~1.0"
    },
"conflict": {
        "symfony/dependency-injection": "<3.4"
    },

See https://github.com/symfony/event-dispatcher/blob/4.3/composer.json

But no matter how we deal with the migration within the Drupal project, we already have a critical issue.
As an example, the upcoming 5.1.0 release of the solarium library implements the migration path as recommended by the symfony project:
https://github.com/solariumphp/solarium/pull/690

This library is required by the search_api_solr module that has more than 14,000 active 8.x installations already. search_api_solr injects the Drupal event dispatcher into the solarium Solr Client.

From my point that works perfectly fine. But Drupal prohibits the update because of this version constraint:
"symfony/event-dispatcher": "~3.4.0",

This version constraint is simply wrong as Drupal doesn't use the event dispatcher provided by this module but only it's EventDispatcherInterface which has been kept compatible in symfony 4.x.

So the current situation is that Drupal blocks feature and security updates of third party libraries which use symfony event dispatching and adopting to symfony event dispatching changes in the proposed and compatible way. That has to be considered as critical.

Proposed resolution

Adjust the version requirement like this:
"symfony/event-dispatcher": "~3.4.0|~4.0"
Afterwards composer will resolve the correct combination of module versions without upgrading the other symfony components at all.
Drupal doesn't use the request dispatcher at all but only implements the EventDispatcherInterface which remains the same in Symfony 4.x compared to 3.4.

Pros Cons
++ Symfony itself declares event-dispatcher 4.3 to be compatible with symfony 3.4 in general - event-dispatcher 4.3 requires PHP >= 7.1 (but for PHP 7.0 the situation doesn't change and composer takes care of this fact)
++ third party libs that follow Symfony's event-dispatcher >= 4.3 migration concept could be used again - if for whatever reason custom code use the symfony event-dispatcher directly they need to follow symfony's migration path or ensure that dependency for event-dispatcher 3.4 is declared correctly in the project's composer.json or even better use Drupal's event dispatcher implementation
++ future security updates of such third party libs must not lead to Drupal specific forks just to backport the fixes
+ contrib modules aren't blocked to leverage new features of newer versions of third party libs
+ we'll discover contrib modules that erroneously use symfony's event dispatcher instead of Drupal's

Remaining tasks

Just apply the patch.

User interface changes

None

API changes

None

Data model changes

None

Release notes snippet

With the release of Symfony 4.3 the event dispatching changed. To unblock Drupal contrib modules to leverage the latest versions of third party libraries that have to deal with that change and to allow composer to install their security releases it is essential that symfony/event-dispatcher 4+ could be installed in Drupal 8. That doesn't mean that Drupal could or would be upgraded to Symfony 4.x entirely. Drupal core itself will still use Symfony 3.4.x and doesn't use Symfony's default event dispatcher but implements it's own.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Eric_A created an issue. See original summary.

Eric_A’s picture

Title: Fix symfony version requirements for drupal/core-event-dispatcher » Fix symfony version requirements in drupal/core-event-dispatcher
Issue summary: View changes
Eric_A’s picture

Issue summary: View changes

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

mkalkbrenner’s picture

Issue summary: View changes
Priority: Normal » Critical
Status: Active » Needs review
FileSize
517 bytes
mkalkbrenner’s picture

Issue summary: View changes
mkalkbrenner’s picture

Title: Fix symfony version requirements in drupal/core-event-dispatcher » Fix symfony version requirements for symfony/event-dispatcher
hchonov’s picture

Something that has been mentioned in #3055194: [Symfony 5] The signature of the "Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher::dispatch()" method should be updated to "dispatch($event, string $eventName = null)", not doing so is deprecated since Symfony 4.3. is that the Symfony Event Dispatcher 4.* requires PHP 7.1 and more specifically PHP 7.1.3. If we are going to update the version constraint to "symfony/event-dispatcher": "~3.4.0|~4.0" then I think that we should raise the required PHP version to 7.1.3, because it would not be possible to install a PHP library having "symfony/event-dispatcher": "~4.0" requirement on a PHP 7.0 environment.

Quoting myself from #3055194-11: [Symfony 5] The signature of the "Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher::dispatch()" method should be updated to "dispatch($event, string $eventName = null)", not doing so is deprecated since Symfony 4.3.:

So why don't we say that from Drupal 8.8.0 we are not going to support PHP 7.0 anymore? That would be exactly 8 months between our promise to support it at least until March 6, 2019 and the release date of Drupal 8.8.0 scheduled for December 4, 2019.

Also PHP 7.0 is EOL since January 10, 2019. I think that 11 months supporting an EOL version is pretty enough, especially given the fact there is an incompatibility that we could fix by doing so.

As with the other issue we require a release manager approval.

mkalkbrenner’s picture

If we are going to update the version constraint to "symfony/event-dispatcher": "~3.4.0|~4.0" then I think that we should raise the required PHP version to 7.1.3, because it would not be possible to install a PHP library having "symfony/event-dispatcher": "~4.0" requirement on a PHP 7.0 environment.

This isn't relevant for this issue. composer handles that.

Nick_vh’s picture

Status: Needs review » Reviewed & tested by the community

Looks like a simple non breaking fix for core and a vital fix that does break a massive ecosystem. It calls again for more coordination between core and the ecosystems but that's for another discussion. Marking this rtbc.

andypost’s picture

Issue tags: +Symfony 4
hchonov’s picture

With the release of Symfony 4.3 the event dispatching changed

If we go with the current solution which allows both versions, then we cannot yet introduce the BC layer and we'll have to wait until we ditch the Symfony 3 Event Dispatcher.

If a release manager agrees with requiring PHP 7.1 as of Drupal 8.8.0 then we could completely switch to the Symfony 4.3 event dispatcher and add the BC layer now and switch to the new behavior in Drupal 9. What do you think about that?

mkalkbrenner’s picture

There’s already another issue for that BC layer PHP Version discussion which I mentioned in the description.

Please don’t mix it with this issue.

Both issues don’t conflict in any way!

But this issue here needs to be fixed quickly to not block security updates of third party libraries.

The other issue is about how drupal itself should deal with the changes Symfony introduced.

And this issue here should ideally be backported to 8.7, too. The other one definitely not.

mikelutz’s picture

This cannot be committed as symfony/event-dispatcher 4 won't pass our core test suite. It would break tests when users composer update.

hchonov’s picture

Only one single core test fails. See #3055194-8: [Symfony 5] The signature of the "Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher::dispatch()" method should be updated to "dispatch($event, string $eventName = null)", not doing so is deprecated since Symfony 4.3.. And this happens because in RedirectResponseSubscriberTest we use the symfony instead our own implementation. Please note that we use the symfony implemention nowhere expect in this test.

Also if custom or contrib code relies on the Symfony 3 event dispatcher implementation, then they should have the corresponding version constraint in their composer.json, which composer will respect and not install the Symfony 4 event dispatcher.

Please note that with #15 I've only wanted to point that we would need a BC layer, but we should do that in a follow-up.

Here we should make it possible for contrib to use the newer event dispatcher if they need it and not prevent them, because we don't have any reasons for doing so.

Therefore I consider #8 RTBC. Could we please reupload the patch and set the issue back to RTBC so that we get a review from a committer?

Status: Needs review » Needs work
mikelutz’s picture

Respectfully, one failing test is sufficient and does need to be addressed before this patch can be committed. One failing test breaks our current vendor max test issue #3044155: PHP7.1 vendor max Testing issue - DO NOT COMMIT and would break the automated vendor max testing that is being worked on by the infrastructure team. One test failure doesn't pass the core testing gate.

Honestly one failure is far fewer than I expected. I realize now that the remaining deprecation errors I expected aren't triggered directly in the event-dispatcher code, they are triggered from the Symfony 4 class loader in conjunction with the event dispatcher annotations, so we are fortunate that we won't see them until we allow the SF4 classloader in Drupal 9. So this may be doable in Drupal 8 once the test failure is addressed.

mikelutz’s picture

hchonov’s picture

Status: Needs review » Reviewed & tested by the community

@mikelutz, thank you for the test adjustments. I think that we're good to go now.

mkalkbrenner’s picture

Thanks for helping here.
The corresponding patch has been committed to drush already :-)
https://github.com/drush-ops/drush/issues/4152

mikelutz’s picture

Priority: Critical » Normal
cspitzlay’s picture

More and more third party code will upgrade to the new event-dispatcher signature and thus require at least 4.3.
Locking people down to event-dispatcher 3.x means cutting them off of security updates in those libraries.
So there is no immediate exploitable hole but security sure is an issue here.

Thus I do not agree that this is just priority "normal".

mikelutz’s picture

Category: Task » Feature request

Event Dispatcher 3.x has security support through November 2021, and we use no third party libraries that depend on any SF4 components (we couldn't, as Drupal 8 supports php 7.0 which cannot install event-dispatcher 4.x)

If we had a third party component in Drupal core depending on event-dispatcher 3 with a security vulnerability that was not going to be patched in the version that was compatible with event-dispatcher 3, then we would indeed have a critical security issue to deal with, but this patch wouldn't fix it, as this only allows event-dispatcher 4 to be installed on systems that have php 7.1 or higher. In that case we would have to fork the library or take other steps to mitigate the security issue.

Since core needs to continue to support php 7.0 through at least the security lifetime of 8.8.x (December 2020), there is nothing in this patch that mitigates any potential security issues, from a drupal core perspective.

This is purely a convenience patch for contrib modules who choose to have a dependency on php7.1 and other third party libraries. These modules would have tighter dependencies than Drupal itself, which they are in their rights to do.

This is a normal priority feature request, not a security issue.

hchonov’s picture

Category: Feature request » Bug report
Priority: Normal » Major
Issue tags: +Contributed project blocker

This is purely a convenience patch for contrib modules

We are not talking about convenience here, but about blocking one of the most used Drupal modules out there. Therefore this is a contributed project blocker. I think that according to https://www.drupal.org/core/issue-priority#major the issue classifies as major bug:

Block contributed projects with no workaround.

I hope that we could agree on that and wait for a core committer's review/reply.

----
I apologise if my interpretation of the issue priority is not correct and/or does not match the views of others.

cspitzlay’s picture

there is nothing in this patch that mitigates any potential security issues, from a drupal core perspective.

This is purely a convenience patch for contrib modules who choose to have a dependency on php7.1 and other third party libraries.

But there is not just Drupal core to consider. Of course the change would mitigate the risk / enable many people to do so by upgrading. The thousands of people who run real-word sites with dependencies on such contrib modules.

For 7.0 users nothing would change but they won't require the update since they won't have such contrib modules in the first place.
I think that denying upgrades (security ones, too) to all rest of the people without even a gain for PHP 7.0 users would be irresponsible.

mkalkbrenner’s picture

Priority: Major » Critical

Meanwhile we have a critical issue in search_api_solr where searching for terms like "d'avion" can cause exceptions when using the Search API HTML Filter: #3076111: Rerank query payload need to be escaped.

This bug is fixed in the required solarium library version 5.1.0: https://github.com/solariumphp/solarium/pull/688
But we can't upgrade to 5.1.0 because the event dispatcher issue is fixed in that version as well, using the symfony 3.4 backward compatible approach recommended by symfony itself.

The only thing required is to accept that simple patch here!

BTW I can't believe that we expect that no other 3rd party library that is required by core or contrib doesn't solve the event dispatcher issue in the same way until the release of Drupal 9 ;-)

mikelutz’s picture

Title: Fix symfony version requirements for symfony/event-dispatcher » Allow symfony/event-dispatcher 4+ to be installed in Drupal 8
Category: Bug report » Feature request
Priority: Critical » Normal
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll
hchonov’s picture

@mikelutz, I refer to #27:

We are not talking about convenience here, but about blocking one of the most used Drupal modules out there. Therefore this is a contributed project blocker. I think that according to https://www.drupal.org/core/issue-priority#major the issue classifies as major bug:

Block contributed projects with no workaround.

I hope that we could agree on that and wait for a core committer's review/reply.

Given the referenced issue priority directives could you please elaborate why you keep changing the issue category to a feature request without an answer to that comment?

Gábor Hojtsy’s picture

Priority: Normal » Major

I don't think it makes a ton of sense to do more status ping-pong here, it does not really matter as long as we can land the issue and unblock contrib modules. Let's agree on a major then? :)

The patch does need a reroll for sure.

mkalkbrenner’s picture

Priority: Major » Normal
Status: Needs work » Needs review
FileSize
3.25 KB
Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Thanks!

mpp’s picture

RTBC++

Thanks!

mikelutz’s picture

I'll respectfully reiterate, and say again that this isn't my decision to make. My opinion below is strictly my own, informed by the work I've been doing over the past 8 months trying to prep the Drupal core codebase to be ready to be compatible with Symfony 4 in Drupal 9.

This is a feature request, not a bug fix, because the decision to restrict Symfony components for version 3 in Drupal 8 is an intentional decision, not a bug. Symfony 4 introduced many incompatible breaking changes across the board. As someone who has spent a great deal of time over the past 8 months working to prepare the core codebase for Symfony 4 compatibility, I know first hand how many gotcha's are in these SF4 releases. It was decided in #2976394: Allow Symfony 4.4 to be installed in Drupal 8, and reconfirmed in DrupalCon meetings, and in many many slack conversations that we are not going to allow Symfony 4 components to be installed in Drupal 8 (without someone taking the manual step of specifying that in their project composer file). This was made for many reasons, the biggest of which is that our testing infrastructure only runs tests against the versions of vendor libraries that ship in our composer.lock file. I showed this earlier by showing the test failure in core that would have occurred if the original patch had been committed. Had we not already been working on preparing the core code base for Drupal 9, there would have been more. Contrib has no way to run their test suite with Symfony 4 components. Currently all of contrib expects that Symfony 3.4.x components will be installed, and with no warning, no change record, and not even a major release, suddenly they have to be compatible with an unexpected SF 4 component. If there is a problem, their tests will show nothing wrong, they will just start getting odd bug reports that won't make any sense or be testable. Sure, core overrides all of the dispatcher except the interface, but who knows what bad practice or hack some contrib module has done that will break by updating the dispatcher? Additionally, Symfony 4 has one more minor release coming out this cycle, Symfony 4.4 in November. In the past Symfony minors have always required some work and adjustments to make work correctly, and historically have cause problems. Again the case may be made that this is unlikely for this particular component, but the possibility remains for Symfony bug fixes, return type adjustments or other allowed changes in the Symfony minor cause problems or expose bugs in core and contrib, again with no way to test or prepare for them. This (along with the lessons learned trying to move from SF2 to SF3 in 8.4) is why we require a Drupal major release to change vendor major releases as a general rule.

It frustrates me because it feels as if there is an attempt to sneak this past the committers as a bug fix instead of what it is: a request to make an exception to the SF3-components-on-Drupal-8 precedent for this one component. While I don't personally believe this change should be made in Drupal 8 due to potential unforseen disruption in contrib (despite writing the patch that could possibly get accepted), that's a release manager decision. You have a reasonable argument to make, in that we override much of this one component anyway, so disruption should be minimal, and I can completely understand how this makes things easier on the Apache Solr project. But you should make that argument on its merits, acknowledge and address the potential disruption rather than pretending that this component is restricted to version 3 because of some typo in the composer.json file that ships with core, and that allowing a major version jump in the component couldn't possibly have any negative side-effects in the ecosystem.

Regarding the priority, I had previously left it at major, but this is definitely not critical, and if I'm going to change it, I have to go with what I believe it should be, which is normal. This does not block a contributed project "with no workaround", I can think of 5 workarounds off the top of my head requiring varying levels of effort.

  1. Fork the latest version of solarium that supports Symfony 3, and backport bug and security fixes as needed
  2. Fork the latest version of solarium and patch it to support Symfony 3
  3. Make a case to and work with the solarium team to maintain security fixes on a branch compatible with SF3 until SF3 EOL
  4. Use an alternative library that does not have requirements that conflict with Drupal Core
  5. Write your own library that does not have requirements that conflict with Drupal Core

BTW I can't believe that we expect that no other 3rd party library that is required by core or contrib doesn't solve the event dispatcher issue in the same way until the release of Drupal 9 ;-)

As I've said before, Apache solr is the only module I can think of that chooses to have a stricter php requirement than Drupal itself. Core and all the rest of contrib must work with PHP 7.0, and therefore must work without Symfony 4 components, or any libraries that would require SF4 components. Apache Solr is a special case, and this is a request for a potentially breaking change that could affect all of contrib to make life easier on one module. Again, the arguments for and against are worth a discussion and review by an RM, but let's do it honestly, and let them make a decision, and not pretend that this is a simple bugfix patch with no side effects, and should obviously be backported etc etc.

So I respectfully beg of all those in favor of making this change, please address the disruption concerns and the seriousness of this change on its merits, and stop pretending that this is a simple obvious bugfix. It's a request for an exception to a rule that could have real possible consequences. Make your case to the RMs, and respect their decision, and I promise you, I will too.

mkalkbrenner’s picture

Regardless how the maintainers will decide here, I feel like I have to express what amount of work is behind a "simple" contrib module and what the side effects are.

I can think of 5 workarounds off the top of my head requiring varying levels of effort.

  1. Fork the latest version of solarium that supports Symfony 3, and backport bug and security fixes as needed
  2. Fork the latest version of solarium and patch it to support Symfony 3
  3. Make a case to and work with the solarium team to maintain security fixes on a branch compatible with SF3 until SF3 EOL
  4. Use an alternative library that does not have requirements that conflict with Drupal Core
  5. Write your own library that does not have requirements that conflict with Drupal Core

Option 4 and 5 will definitely not happen. We put a lot of energy into joining forces to contribute to a common library! Different frameworks dropped their self made implementations and leverage solarium today - and are contributing to it!

Solarium is used by latest integration modules for Drupal, Typo3, Wordpress, silverstripe, ...
And there's a solarium bundle for symfony and an older zend integration has been declared deprecated in favor of solarium.
According to packagist there's no other relevant PHP library for Solr anymore and the framework integration packages just leverage solarium:
https://packagist.org/?query=solr

And solarium seems to be the only library that officially supports Solr 7 and 8 and Solr Cloud.

As you might know - and mainly because of Drupal and Search API - I became one of the solarium maintainers, too. So I can tell that option 3 will also not happen. In fact it was me who blocked to raise the dependency to PHP 7.1 back in December 2017 because of Drupal.

But when the solarium 5.0 development started, PHP 7.0 already was close to EOL and "Drupal" wasn't strong enough as an argument. So we moved on to PHP 7.1. BTW that's the reason why search_api_solr 8.x-1.x is still supported until December 2019, to have an option for Drupal people that are still on PHP 7.0. But these releases use an unmaintained older version of solarium.

And finally nobody would pay for option 1,2 or 3.

So the only "workaround" I can think of to get the pending bugfixes for search_api_solr committed that depend on bugfixes in solarium 5.1.x is to open a search_api_solr 8.x-4.x branch and raise the solarium version there. And in the installation instructions for that branch will point to the core patch here and finally look like this:

composer require symfony/event-dispatcher:"4.3.3 as 3.4.99" drupal/search_api_solr:^4.0

But I consider that to be a bad choice.

Anyway I really respect miklutz' work on the symfony task in general! Nevertheless i have some comments:

Currently all of contrib expects that Symfony 3.4.x components will be installed, and with no warning, no change record, and not even a major release, suddenly they have to be compatible with an unexpected SF 4 component.

I agree on most of the argumentation in #36. But the event-dispatcher case is special and could not be treated as Symfony 4 in general.

Contrib has no way to run their test suite with Symfony 4 components.

That's not true. Search API Solr is already tested including the 4.3 event dispatcher: https://travis-ci.org/mkalkbrenner/search_api_solr/builds/580861979
(The current test failures are caused by the mink issue which we see in the "stable" 3.x branch as well!)

Currently all of contrib expects that Symfony 3.4.x components will be installed, and with no warning, no change record, and not even a major release, suddenly they have to be compatible with an unexpected SF 4 component.

Again, that's not true in general. I'm only talking about the event dispatcher. Contrib modules should not leverage the symfony event dispatcher directly. They should use the drupal event dispatcher which implements the event dispatcher interface.
If - for what ever reason - a contrib module directly creates a new symfony event dispatcher it has to declare that dependency in it's composer.json. But I don't know any contrib module that's not leveraging the drupal event dispatcher.

Quite the contrary search_api_solr injects the drupal event dispatcher into solarium which causes solarium not to use the symfony event dispatcher directly. But in this case solarium decorates the drupal event dispatcher with the compatibility LegacyProxy provided by symfony 4.3.

So I still vote for following the symfony recommendation about how to deal with the event-dispatcher issue when using 'old' and 'new' dispatching in parallel.

mikelutz’s picture

I absolutely respect the work that @mkalkbrenner and others have done with the solr module and with the solarium library. I very much understand the difficult position you are in, with attempting to maintain the solr module given the Drupal 8 vendor requirements.

I have voiced my concerns and do not intend to comment on this issue further. I do hope you are able to get a satisfactory resolution to the issue implemented whether that is this patch or something better than none of us have thought of yet.

Best regards and good luck.

xjm’s picture

Category: Feature request » Task
Priority: Normal » Major
Issue tags: -Needs reroll +Needs issue summary update, +Needs release note

Thanks everyone for your detailed input here.

So this is indeed a major task as a contributed project blocker or perhaps a soft-blocker as there are workarounds (even if they're not great ones). It's not critical but also not really a bug per se. Let's not tug-of-war the issue priority further or demote it again.

I've not had the opportunity to read through all the pros and cons above, which we'll need to take into account when deciding whether we should commit this. An issue summary update listing the pros and cons of this fix would help. The issue would need a CR and maybe a release note explaining the potential impacts of the change on sites and modules, and those things would also help with evaluating this issue.

Thanks!

xjm’s picture

catch’s picture

I haven't completely ruled out #3020303: Consider using Symfony flex to allow switching between major Symfony versions still (see the symfony/flex version from #38 onwards) but not sure how much if at all that issue would help here (were we actually to decide to do it).

hchonov’s picture

#3020303: Consider using Symfony flex to allow switching between major Symfony versions will solve the issue here, because it allows for the Symfony4 event dispatcher to be installed.

However in the issue here a problem with the test RedirectResponseSubscriberTest was discovered when using the Symfony 4 event dispatcher, which is being fixed by the provided patch already.

I think that we could consider this particular case in isolation regardless of the decision whether to allow all symfony 4 components to be installed. However the decision here could influence the much bigger issue of allowing all symfony components to be installed in version 4.

catch’s picture

@xjm suggested this in chat but I agree - can we split of the test changes to a follow-up since those definitely need to happen regardless of everything else?

hchonov’s picture

Extracting the test changes into a separate issue makes perfect sense, however according to #2876675-20: Allow symfony/event-dispatcher 4+ to be installed in Drupal 8 I am not sure if @mikelutz will agree with doing this in a follow-up:

Respectfully, one failing test is sufficient and does need to be addressed before this patch can be committed. One failing test breaks our current vendor max test issue #3044155: PHP7.1 vendor max Testing issue - DO NOT COMMIT and would break the automated vendor max testing that is being worked on by the infrastructure team. One test failure doesn't pass the core testing gate.

So maybe it should happen as a prerequisite of the current issue.

catch’s picture

Sorry that's what I meant - a spin-off pre-requisite as opposed to a follow-up.

mkalkbrenner’s picture

mkalkbrenner’s picture

.

mkalkbrenner’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Needs review
Issue tags: -Needs issue summary update
FileSize
517 bytes

Here's the adjusted and simplified patch due to the commit of #3081582: EventSubscriber/RedirectResponseSubscriberTest must not use the Symfony EventDispatcher but Drupal's.

I updated the issue summery, but I don't know if it meets your requirements.

Due to the fact that the change is transparent to the user / developer I'm not sure what to provide as release note or change record. Only that it is allowed to install symfony/event-dispatcher 4.x now and that composer will decide on it?

mkalkbrenner’s picture

.

mkalkbrenner’s picture

Issue summary: View changes
mkalkbrenner’s picture

Issue summary: View changes
mkalkbrenner’s picture

Issue tags: -Needs change record
hchonov’s picture

Status: Needs review » Reviewed & tested by the community

Everything that was asked for is done and I think that we are ready to go back to RTBC now. Thank you, @mkalkbrenner.

bojanz’s picture

Repeating my comment from #3020303: Consider using Symfony flex to allow switching between major Symfony versions since we seem to be duplicating that issue:

Echoing berdir's #24. This feels extremely unwise.

The documented starting point for every Drupal Commerce site is "composer create-project".
This will silently switch a large portion of our users to Symfony 4. That makes it the equivalent of Drupal core switching the tarball to Symfony 4.

It is worse to have some people on Symfony 4 (and experiencing contrib incompatibilities) and some on Symfony 3, than to just move everyone to Symfony 4.

Allowing vastly different Symfony versions to be used based on the PHP version and used contribs is a recipe for instability. So I am -1 on this issue just like I am -1 on #3020303: Consider using Symfony flex to allow switching between major Symfony versions.

I understand that this creates problems for Search API Solr maintainers who want to use the latest library version, instead of staying on an older one, but having a PHP library depend on Symfony will always be a cause of breakage. Dependent projects have very different paces of adopting Symfony versions. This is why we've worked hard to avoid such dependencies in the commerceguys libraries, ensuring Symfony dependencies stay in Symfony bundles.

mkalkbrenner’s picture

The best solution would be to just use symfony/event-dispatcher 4.3 in core while keeping any other symfony component on 3.4.
The only thing that prevents us from doing so is the support for PHP 7.0 we keep regardless that it reached EOL.

Allowing vastly different Symfony versions to be used based on the PHP version and used contribs is a recipe for instability.

The proposed solution here works perfectly well with composer. Since symfony/event-dispatcher 4.3. is declared to be compatible with symfony 3.4 in general and due to the fact that drupal doesn't use anything but the dispatcher interface (not any class or code) I don't think that this statement applies here.

hchonov’s picture

Re #54:

Repeating my comment from #3020303: Consider using Symfony flex to allow switching between major Symfony versions since we seem to be duplicating that issue

In this issue we are talking about switching to a newer version of a single component and not all components, therefore saying that we are repeating #3020303: Consider using Symfony flex to allow switching between major Symfony versions is not really correct.

This switch here is safe, because Drupal uses only the Interface, but offers its own implementation of the event dispatcher. Also the changes in the v4 version of the event dispatcher have been documented here and there have been specifically made with BC in mind. For Drupal and its own implementation of the event dispatcher there is absolutely no difference whether we use the v3 or v4 symfony event dispatcher.

It would be really nice if people against this change give an example of something that will go wrong by switching to the v4 event dispatcher instead of only claiming that

This feels extremely unwise

and

Allowing vastly different Symfony versions to be used based on the PHP version and used contribs is a recipe for instability.

.

Spokje’s picture

FWIW: Our agency has been using symfony/event-dispatcher 4.3 on 4 of our 25 D8 sites since the release of search_api_solr 3.5 without any issues whatsoever.

These sites have a wide array of different Contrib and Custom modules installed. As said: No problems were detected.

We will be using it on our other 6 Solr-using D8 sites shortly.

KarimB’s picture

FYI: I'v been using symfony/event-dispatcher 4.3.4 since the release of search_api_solr 3.6 without any issue so far. It's now in production on https://www.videodrupal.org/ and everything seems working well.

TrevorBradley’s picture

Hoping this gets resolved soon and backported to 8.7.x!

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

mkalkbrenner’s picture

Issue tags: -Needs release note

Since everything requested by catch and xjm was already provided a month ago, it would be very nice if a maintainer could go on here.
According to the search_api_solr usage statistics we know that more than 500 sites must have switched to symfony/event-dispatcher 4.3.x already!

jibran’s picture

+++ b/core/composer.json
@@ -21,7 +21,7 @@
-        "symfony/event-dispatcher": "~3.4.0",
+        "symfony/event-dispatcher": "~3.4.0|~4.0",

We are changing the composer.json file we need to update the composer.lock as well. Please run composer update drupal/core after applying the patch.

mikelutz’s picture

There is nothing to change in the lock file, if we make this change we are still shipping with the same version as before.

mkalkbrenner’s picture

@mikelutz: I agree. Can you set RTBC again?

xjm’s picture

Issue tags: +Needs release note

We document any changes to our dependency constraints and there is some chance of disruption, so please add the release note. Thanks!

mkalkbrenner’s picture

We document any changes to our dependency constraints and there is some chance of disruption, so please add the release note. Thanks!

I thought I did that with https://www.drupal.org/node/3081702

If a "release note" is something different, how do I create one?

mikelutz’s picture

That's a change record. The release note is the snippet in the issue summary that currently says:

Release notes snippet

Todo, maybe not required at all.

nicrodgers’s picture

There's some guidance on how to write a release note here:
https://www.drupal.org/issue-summaries#release-notes

If I understood the issue better I'd attempt to write one, sorry!

mkalkbrenner’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs release note
hchonov’s picture

Status: Needs review » Reviewed & tested by the community

Now that this was resolved.

Status: Reviewed & tested by the community » Needs work
Spokje’s picture

Status: Needs work » Reviewed & tested by the community

Back to RTBC after unexpected TestBot failure wasn't triggered on second run with the same patch #48

davide.porrovecchio’s picture

@mkalkbrenner I can't figure out how to apply this patch in my local Drupal instance, can you help me please?

After the patch is applied, the composer.json is modified but this change does not affect the composer update command, as this is based on the contents of the composer.lock file.

composer prohibits symfony/event-dispatcher 4.x
drupal/core 8.8.2 requires symfony/event-dispatcher (~3.4.0)

I'm new to Drupal so maybe I didn't understand how to use patches, if so please be patient.

Thanks

ecvandenberg’s picture

I'm certainly not new to Drupal and even not new to Composer, but also struggling to get the patch in action.
@davide.porrovecchio the composer update command does not look at the composer.lock file. It ignores it. The lock file is used by composer install.

But even with these commands composers reports there is nothing to install or update.:

  • composer update
  • composer update drupal/core
  • composer update drupal/core --with-all-dependencies
  • composer update drupal/search_api_solr --with-all-dependencies

I'm aware this actually is a composer question but I guess I'm not the only one struggling. Can someone give us a hint.

mkalkbrenner’s picture

The "problem" with the patch is, that it is not recognized when you manage drupal itself via composer. Therefore has to be committed to the repository!

If you want to test it locally you need to declare a git checkout of drupal with the patch applied as a local composer repository.

Berdir’s picture

I can't speak for the release/framework maintainers but I think at this point it's pretty much guaranteed that this won't be committed anymore. Drupal 8.9 and 9.0 will come out at the same time and are now in beta, there is a workaround through the alias (composer require symfony/event-dispatcher:"4.3.3 as 3.4.99") and we did as much as we could to allow people to update to 8.8/8.9 safely, there are almost no new updates in 8.9/9.0, we fixed upgrade path bugs and so on so that people can get on the Drupal 8 LTS release. This would introduce some risk, as small as it might be. We've been using the workaround too now as 3.x is the only supported release and I'm not happy about it, but I think it's something we can live with until we can update to Drupal 9.

andypost’s picture

It looks too late for 8.9 and missing release manager review

OTOH related #2825358: Event class constants for event names can cause fatal errors when a module is installed better fits into 9.1 and better coordinate them

mikelutz’s picture

Status: Reviewed & tested by the community » Closed (won't fix)

This missed the beta cutoff for 8.9, and is already fixed in 9.0

xjm’s picture

Yeah, this was too disruptive. Thanks!

cspitzlay’s picture

reroll against 8.9 for those who need it.

sam452’s picture

Coming late to this on our 8.9.1 production install. First, I modified web/core/composer.json using the patch @cspitzlay provided. Ran composer update drupal/core.

drupal/core-recommended 8.9.1 requires symfony/event-dispatcher v3.4.41 -> satisfiable by symfony/event-dispatcher[v3.4.41] but these conflict with your requirements or minimum-stability.

Removed composer.lock, vendor folder and ran composer install.

- drupal/core-recommended 8.9.1 requires symfony/event-dispatcher v3.4.41 -> satisfiable by symfony/event-dispatcher[v3.4.41] but these conflict with your requirements or minimum-stability.

Running composer require symfony/event-dispatcher:"4.3.3 as 3.4.99" returns

- drupal/core-recommended 8.9.1 requires symfony/event-dispatcher v3.4.41 -> satisfiable by symfony/event-dispatcher[v3.4.41] but these conflict with your requirements or minimum-stability.

Re-reading @mkalkbrenner comment in https://www.drupal.org/project/drupal/issues/2876675#comment-13517660. Are you saying I should find a copy of the symfony/event-dispatcher 4.3 and drop it into my vendor folder? I've read these comments many times and it's not clear how to get past this issue without migrating to drupal 9.0.

Berdir’s picture

You are using drupal/core-recommended, which requires a specific patch version of symfony dependencies. Your require as command needs to match that exact version. You might also want to use the latest available version of 4.3 as source. When using drupal/core-recommended, you will need to keep this in sync with drupal core in case updates there update that version.

sam452’s picture

Thank you for your prompt answer. I accept what you're saying is true, but I don't see how to apply.

You are using drupal/core-recommended, which requires a specific patch version of symfony dependencies. Your require as command needs to match that exact version.

It's not obvious to me how the suggested command composer require symfony/event-dispatcher:"4.3.3 as 3.4.99" can be adapted to handle drupal/core-recommended specifically as I'm hearing. Where would I find the specific patch version of symfony dependencies that I need with drupal/core-recommended?

You might also want to use the latest available version of 4.3 as source.

Are we explicitly saying to download the source code from here and drop it in the vendor directory? That doesn't seem right as I find that web/core/lib/Drupal/Component/EventDispatcher/ContainerAwareEventDispatcher.php is unable to find 'Symfony\Component\EventDispatcher\EventDispatcherInterface'.

Berdir’s picture

You basically already answered your own question, you wrote:

Running composer require symfony/event-dispatcher:"4.3.3 as 3.4.99" returns

- drupal/core-recommended 8.9.1 requires symfony/event-dispatcher v3.4.41 -> satisfiable by symfony/event-dispatcher[v3.4.41] but these conflict with your requirements or minimum-stability.

drupal/core-recommended wants 3.4.41, not 3.4.99, so just use "as 3.4.41".

And you can find the latest 4.3.x version of event manager on https://packagist.org/packages/symfony/event-dispatcher, so that would be 4.3.11, so the result is "4.3.11 as 3.4.41". That's it, no manual copying of anything, simple as that.

Update: The not-so-simple part is then when you want to update drupal core to the latest 8.9 version, and maybe that will require symfony 3.4.42, then it won't update because that would conflict with you requiring 3.4.41. So you will need to check updated core versions and adjust that definition. But you should at this point just look into updating to D9 soon, where it will just work.

Yanivs’s picture

In case you want to install 8.9.x and receive this event-dispatcher error, you need to run composer require for both drupal core and event-dispatcher.

This is an example for updating Drupal core (core-recommended) to 8.9.20 together with the event-dispatcher dependency:

composer require drupal/core-recommended:8.9.20 symfony/event-dispatcher:"4.4.34 as 3.4.41" --update-with-all-dependencies

Solved my problem, maybe it will solve yours.