The ultimate goal of this issue is to remove \Drupal\Core\Messenger\LegacyMessenger
entirely (which is deprecated and scheduled for removal prior to 9.0.0) and solely rely on the Messenger service.
If possible, 3rd party code should use the Messenger service or find alternative solutions that don't involve using this deprecated class.
If there is any 3rd party code that provides a legitimate use case for keeping this around, we may have to re-think how messages work (highly unlikely, but currently a complete unknown).
This is partially due to the fact that the drupal_set_message()
and drupal_get_messages()
proceedural functions have been around for over 14 years and usage of these functions was heavy throughout core, contrib and 3rd party code alike.
Thus, this issue also serves as a way to track usage of this deprecated class.
Use cases:
- Drush - Used when creating a new site and the container is not yet initialized. Unclear if this is a hard dependency or its code can be fixed, needs research
Comment | File | Size | Author |
---|---|---|---|
#20 | 2928994-20.patch | 15.47 KB | alexpott |
#20 | 15-20-interdiff.txt | 3.37 KB | alexpott |
#15 | 2928994-15-interdiff.txt | 2.1 KB | Berdir |
#15 | 2928994-15.patch | 13.86 KB | Berdir |
#5 | 2928994-5.patch | 11.77 KB | JeroenT |
Comments
Comment #2
markhalliwellComment #3
martin107 CreditAttribution: martin107 as a volunteer commentedHere is a 8.5.x change record
https://www.drupal.org/node/2774931
It was committed to 8.5.x today
#2760167: Add \Drupal\Core\Messenger\Messenger
Comment #5
JeroenTPatch attached removes the LegacyMessenger, MessengerLegacyTest and replaces the usage of LegacyMessenger in \Drupal::messenger().
Comment #6
JeroenTComment #7
borisson_The patch in #5 seems to do exactly what this issue is asking for.
Comment #9
JeroenTComment #10
JeroenTHmm, strange that "Module %module has been enabled." is no longer shown after installing a module. I started debugging the test failures but I honestly have no clue what's going on here.
Discussed with @borisson_ and @wimleers. One of the possible reasons is that the ModuleInstaller is using it's own container.
If someone else want's to give this one a go, feel free. #famouslastwords
Comment #11
BerdirI suspect what happens is that the container rebuild means we lose the stored messages in there. We might need to do something like \Drupal\Core\Extension\ModuleInstaller::updateKernel() so that we move the stored messages from the old to the new container?
Comment #12
pratik_kambleComment #13
pratik_kambleComment #14
andypostWorkaround #11 looks like a way!
Related #2473875: Convert uses of $_SESSION to symfony session retrieved from the request and its child issues should fix it too
Comment #15
BerdirOk, I think it is even weirder than that.
I made the change that I suggested, directly in DrupalKernel, but that didn't work yet. Then I realized that it only failed on experimental modules. The problem actually is because $this->messenger had been initialized before in ModulesListConfirmForm, so we ended up accessing the service from the old container and added our message there.
As a quickfix, I just unset it now. Rebuilding the container within a running system is a funny business, as usual.
Comment #16
BramDriesenFunny business indeed :-) But all tests passed so I guess it should be okay. Deprecation deletions look okay.
Comment #17
alexpottThis logic makes me think a couple of things.
I think we really should explore having a micro-container that cannot change during a request and is a solid base for Drupal to stand on.
My other thought that is more actionable is that we're storing messages in the
session.flash_bag
service. So does that mean our session save saves them and if so is this really necessary - and if it is necessary then maybe we're preserving the wrong thing - i.e. should we be preserving just the state of the page cache kill switch or should we be preserving the state of thesession.flash_bag
service.Comment #18
BerdirI guess we can probably skip that part and only keep the change in the confirm form?
Comment #19
alexpottYep I think that works - at least it does locally. I wonder if we have test coverage of a message surviving a container rebuild.
Comment #20
alexpottPatch removes the changes to DrupalKernel and adds a test to ensure that messages survive a container rebuild.
Comment #21
jibranCan someone please test this manually with
drush site:install
as well? Not adding theLegacyMessenger
first time broke thedrush si
.Comment #22
BerdirWell, LegacyMessenger has been a BC layer, so it's drush that will need to change if it breaks now? Besides, drush on D9 is broken for me anyway atm.
Comment #23
alexpottTested on Drush 10 and it works fine - both minimal and standard.
Comment #24
BerdirNice, setting back to RTBC. I added those changes to try and fix that test, before realizing that the real problem was elsewhere.
Comment #26
catchCommitted 89d0409 and pushed to 9.0.x. Thanks!