Remove \Drupal\Core\Messenger\LegacyMessenger

Here is a 8.5.x change record

drupal_get_message() and drupal_set_message() replaced by Messenger service

https://www.drupal.org/node/2774931

It was committed to 8.5.x today

#2760167: Add \Drupal\Core\Messenger\Messenger

Patch attached removes the LegacyMessenger, MessengerLegacyTest and replaces the usage of LegacyMessenger in \Drupal::messenger().

The patch in #5 seems to do exactly what this issue is asking for.

Hmm, 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

I 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?

Workaround #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

Ok, 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.

Funny business indeed :-) But all tests passed so I guess it should be okay. Deprecation deletions look okay.

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -861,6 +861,7 @@ protected function getKernelParameters() {
+    $all_messages = [];
     if (isset($this->container)) {
       // Save the id of the currently logged in user.
       if ($this->container->initialized('current_user')) {
@@ -876,6 +877,8 @@ protected function initializeContainer() {

@@ -876,6 +877,8 @@ protected function initializeContainer() {
         }
         unset($session);
       }
+
+      $all_messages = $this->container->get('messenger')->all();
     }
 
     // If we haven't booted yet but there is a container, then we're asked to
@@ -936,6 +939,13 @@ protected function initializeContainer() {

@@ -936,6 +939,13 @@ protected function initializeContainer() {
       $this->container->get('current_user')->setInitialAccountId($current_user_id);
     }
 
+    // Re-add messages.
+    foreach ($all_messages as $type => $messages) {
+      foreach ($messages as $message) {
+        $this->container->get('messenger')->addMessage($message, $type);
+      }
+    }

This 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 the session.flash_bag service.

I guess we can probably skip that part and only keep the change in the confirm form?

Yep I think that works - at least it does locally. I wonder if we have test coverage of a message surviving a container rebuild.

Patch removes the changes to DrupalKernel and adds a test to ensure that messages survive a container rebuild.

Can someone please test this manually with drush site:install as well? Not adding the LegacyMessenger first time broke the drush si.

Well, 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.

Tested on Drush 10 and it works fine - both minimal and standard.

Nice, setting back to RTBC. I added those changes to try and fix that test, before realizing that the real problem was elsewhere.

Committed 89d0409 and pushed to 9.0.x. Thanks!