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.
Comments
Comment #2
Eric_A CreditAttribution: Eric_A commentedComment #3
Eric_A CreditAttribution: Eric_A commentedComment #8
mkalkbrennerComment #9
mkalkbrennerComment #10
mkalkbrennerComment #11
hchonovSomething 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.:
As with the other issue we require a release manager approval.
Comment #12
mkalkbrennerThis isn't relevant for this issue. composer handles that.
Comment #13
Nick_vhLooks 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.
Comment #14
andypostComment #15
hchonovIf 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?
Comment #16
mkalkbrennerThere’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.
Comment #17
mikelutzThis cannot be committed as symfony/event-dispatcher 4 won't pass our core test suite. It would break tests when users composer update.
Comment #18
hchonovOnly 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?
Comment #20
mikelutzRespectfully, 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.
Comment #21
mikelutzHere is an update to the problem test to use our dispatcher instead of Symfony's.
I'm running the tests with and without the updated dependency installed. If they are both green, then we pass the testing gate.
Comment #22
hchonov@mikelutz, thank you for the test adjustments. I think that we're good to go now.
Comment #23
mkalkbrennerThanks for helping here.
The corresponding patch has been committed to drush already :-)
https://github.com/drush-ops/drush/issues/4152
Comment #24
mikelutzThis issue doesn't meet the criteria to be classified as 'critical'
Comment #25
cspitzlayMore 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".
Comment #26
mikelutzEvent 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.
Comment #27
hchonovWe 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:
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.
Comment #28
cspitzlayBut 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.
Comment #29
mkalkbrennerMeanwhile 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 ;-)
Comment #30
mikelutzComment #31
hchonov@mikelutz, I refer to #27:
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?
Comment #32
Gábor HojtsyI 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.
Comment #33
mkalkbrennerComment #34
Gábor HojtsyThanks!
Comment #35
mpp CreditAttribution: mpp at AmeXio for District09 commentedRTBC++
Thanks!
Comment #36
mikelutzI'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.
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.
Comment #37
mkalkbrennerRegardless 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.
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:
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:
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.
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!)
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.
Comment #38
mikelutzI 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.
Comment #39
xjmThanks 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!
Comment #40
xjmComment #41
catchI 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).
Comment #42
hchonov#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.
Comment #43
catch@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?
Comment #44
hchonovExtracting 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:
So maybe it should happen as a prerequisite of the current issue.
Comment #45
catchSorry that's what I meant - a spin-off pre-requisite as opposed to a follow-up.
Comment #46
mkalkbrennerHere's the pre-requisite as requested: #3081582: EventSubscriber/RedirectResponseSubscriberTest must not use the Symfony EventDispatcher but Drupal's.
Comment #47
mkalkbrenner.
Comment #48
mkalkbrennerHere'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?
Comment #49
mkalkbrenner.
Comment #50
mkalkbrennerComment #51
mkalkbrennerComment #52
mkalkbrenneradded change record: https://www.drupal.org/node/3081702
Comment #53
hchonovEverything that was asked for is done and I think that we are ready to go back to RTBC now. Thank you, @mkalkbrenner.
Comment #54
bojanz CreditAttribution: bojanz at Centarro commentedRepeating my comment from #3020303: Consider using Symfony flex to allow switching between major Symfony versions since we seem to be duplicating that issue:
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.
Comment #55
mkalkbrennerThe 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.
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.
Comment #56
hchonovRe #54:
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
and
.
Comment #57
SpokjeFWIW: Our agency has been using
symfony/event-dispatcher
4.3 on 4 of our 25 D8 sites since the release ofsearch_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.
Comment #58
KarimB CreditAttribution: KarimB as a volunteer commentedFYI: 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.
Comment #59
TrevorBradley CreditAttribution: TrevorBradley commentedHoping this gets resolved soon and backported to 8.7.x!
Comment #61
mkalkbrennerSince 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!
Comment #62
jibranWe are changing the
composer.json
file we need to update thecomposer.lock
as well. Please runcomposer update drupal/core
after applying the patch.Comment #63
mikelutzThere is nothing to change in the lock file, if we make this change we are still shipping with the same version as before.
Comment #64
mkalkbrenner@mikelutz: I agree. Can you set RTBC again?
Comment #65
xjmWe document any changes to our dependency constraints and there is some chance of disruption, so please add the release note. Thanks!
Comment #66
mkalkbrennerI thought I did that with https://www.drupal.org/node/3081702
If a "release note" is something different, how do I create one?
Comment #67
mikelutzThat'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.
Comment #68
nicrodgersThere'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!
Comment #69
mkalkbrennerComment #70
hchonovNow that this was resolved.
Comment #72
SpokjeBack to RTBC after unexpected TestBot failure wasn't triggered on second run with the same patch #48
Comment #73
davide.porrovecchio CreditAttribution: davide.porrovecchio as a volunteer commented@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 thecomposer update
command, as this is based on the contents of thecomposer.lock
file.I'm new to Drupal so maybe I didn't understand how to use patches, if so please be patient.
Thanks
Comment #74
ecvandenberg CreditAttribution: ecvandenberg as a volunteer commentedI'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.:
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.
Comment #75
mkalkbrennerThe "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.
Comment #76
BerdirI 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.Comment #77
andypostIt 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
Comment #78
mikelutzThis missed the beta cutoff for 8.9, and is already fixed in 9.0
Comment #79
xjmYeah, this was too disruptive. Thanks!
Comment #80
cspitzlayreroll against 8.9 for those who need it.
Comment #81
sam452 CreditAttribution: sam452 as a volunteer commentedComing 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.
Removed composer.lock, vendor folder and ran composer install.
Running
composer require symfony/event-dispatcher:"4.3.3 as 3.4.99"
returnsRe-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.
Comment #82
BerdirYou 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.
Comment #83
sam452 CreditAttribution: sam452 as a volunteer commentedThank you for your prompt answer. I accept what you're saying is true, but I don't see how to apply.
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?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'.
Comment #84
BerdirYou basically already answered your own question, you wrote:
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.
Comment #85
Yanivs CreditAttribution: Yanivs as a volunteer commentedIn 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.