Problem/Motivation

When a new module is installed (kernel rebuild) the reference between the messenger service and the flashbag is reset. Any messenger service instantiated or any messages set before the this will be lost. See #474684-141: Allow themes to declare dependencies on modules

Also messages set in the early bootstrap are lost as well. Both of these are a regression introduced by #2760167: Add \Drupal\Core\Messenger\Messenger

Proposed resolution

Instead of injecting session.flash_bag into the messenger inject session so that when the messenger service adds a message the session is started.

Remaining tasks

User interface changes

None

API changes

In \Drupal\Core\Messenger\Messenger::__construct() change FlashBagInterface to SessionInterface

Data model changes

None

CommentFileSizeAuthor
#62 interdiff_61_62.txt4.98 KBAbhisheksingh27
#62 2940148-62.patch12.66 KBAbhisheksingh27
#61 2940148-61.patch10.51 KBmstrelan
#56 2940148-56.patch11.62 KBnikitagupta
#55 2940148_91x.patch12.66 KBjjpilcher
#50 2940148-2-50.patch12.4 KBalexpott
#50 48-50-interdiff.txt1.73 KBalexpott
#48 2940148-48.patch11.89 KBangelamnr
#43 2940148-43.patch11.75 KBalexpott
#43 2940148-43.test-only.patch1.68 KBalexpott
#40 2940148-40.patch10.07 KBalexpott
#40 2940148-40.test-only.patch3.62 KBalexpott
#40 36-40-interdiff.txt4.36 KBalexpott
#36 2940148-36.patch6.38 KBalexpott
#36 33-36-interdiff.txt1.79 KBalexpott
#33 2940148-33.patch5.9 KBalexpott
#33 31-33-interdiff.txt1.55 KBalexpott
#31 26-31-interdiff.txt5.05 KBalexpott
#31 2940148-31.patch5.67 KBalexpott
#26 interdiff-2940148-20-26.txt2.65 KBkevineinarsson
#26 2940148-26.patch7.42 KBkevineinarsson
#21 interdiff-2940148-19-20.txt6.14 KBkevineinarsson
#21 2940148-20.patch8.02 KBkevineinarsson
#19 interdiff-2940148-14-19.txt2.35 KBkevineinarsson
#19 2940148-19.patch6.3 KBkevineinarsson
#14 2940148-14.patch3.78 KBkevineinarsson
#11 2940148-11.patch4.99 KBdawehner
#10 2940148-10-complete.patch3.8 KBkevineinarsson
#10 2940148-10-tests.patch783 byteskevineinarsson
#7 2940148-7.patch2.04 KBkevineinarsson
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

kalpaitch created an issue. See original summary.

kalpaitch’s picture

Issue summary: View changes
larowlan’s picture

Priority: Normal » Major

This is arguably a form of data loss

dillix’s picture

@larowlan should it be backported to 8.5, or we can live without it till 8.6?

larowlan’s picture

Bugs are always eligible for backport

markhalliwell’s picture

We actually semi-ran into this before https://www.drupal.org/project/drupal/issues/2760167#comment-12368450

Did a quick search and found https://stackoverflow.com/a/45498223

Maybe we're just not injecting the correct flashbag?

Currently, it uses @session.flash_bag, but if the above SO answer is any indication... perhaps it should be tied to the request's session flash bag instead?

kevineinarsson’s picture

Patch attached adds an event subscriber directly to the Messenger service. It also flags the messenger service as persistent. Otherwise a new service would be created because the event is dispatched after the container rebuild.

kevineinarsson’s picture

Status: Active » Needs review
markhalliwell’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests
kevineinarsson’s picture

Needs a bootstrapped browser test / a better test because this doesn't make sure that old messages are being carried over.

dawehner’s picture

I'm not convinced of this solution: #2368263: Remove the persist service tag makes this pretty clear.
This solution might help ...

Status: Needs review » Needs work

The last submitted patch, 11: 2940148-11.patch, failed testing. View results

kevineinarsson’s picture

Ah, okay. Didn't know about that. My patch had a typo anyways -- let's just hide that.

kevineinarsson’s picture

Using the approach in #11 but using the Session service instead of the Session Manager service works... Still no tests.

kevineinarsson’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 14: 2940148-14.patch, failed testing. View results

dawehner’s picture

@kevineinarsson
Sorry for not responding per email. Email just doesn't work nicely for me :(
I think the next steps you could do is to figure out where these remaining test failures are coming from. Do you have an idea already?

kevineinarsson’s picture

The tests seem to be failing because of the new dependency chain leading to the database service via the session manager service. So each test that calls drupal_get_message() or tries to add a message fails with a missing database connection. I'm not sure where or how to fix this.

No prob on the email! I just went ahead and did something. :) I emailed mainly not to spam the issue node with useless unrelated questions.

kevineinarsson’s picture

Status: Needs work » Needs review
FileSize
6.3 KB
2.35 KB

Does something like this make sense? Should fix the service trying to spin up the database before db setup.

Status: Needs review » Needs work

The last submitted patch, 19: 2940148-19.patch, failed testing. View results

kevineinarsson’s picture

Right, might be a good idea not to break being able to add and display messages before the database is available... How about using an in-memory messenger until the database has been verified and the normal messenger service can be spun up? Sorta what the installer does but simpler. Might we lose some messages here..?

Status: Needs review » Needs work

The last submitted patch, 21: 2940148-20.patch, failed testing. View results

markhalliwell’s picture

The test from #10 has been lost in the mix.

Also, I do not understand what the latest patches are trying to accomplish. There is already an "in-memory" messenger of sorts with LegacyMessenger. This issue is specifically about the container rebuild. I still think my comment in #6 may have some relevance.

kevineinarsson’s picture

@markcarver, This is related to the main issue in a sense that changing the route to access the flashbag introduces other failure points. Using either the session or session_manager service, a database connection needs to be available. Therefore, in environments where no database has been defined (like when installing Drupal for the first time), the Messenger service won't be able to be instantiated.

As a sidenote, fetching the session through the request_stack service would also fail in this state as a session isn't saved to it before a full bootstrap is performed (basically after the DB step in the installer as I understand it).

If we were to alter the getMessengerService() method on the LegacyMessenger, we could catch the DatabaseNotFoundException that's thrown and just continue with the static messenger. But that would only solve legacy calls. To also cover calls to \Drupal::service('messenger'), overriding the definition of the messenger service seems to be the way to go.

kevineinarsson’s picture

@dawehner, @markcarver: Thoughts?

kevineinarsson’s picture

Status: Needs work » Needs review
FileSize
7.42 KB
2.65 KB

Finally I managed to get xdebug working with PHPUnit...

So I can finally start digging into writing a proper test. As I suspected however the test I had originally submitted in #10 is borked. In CLI sessions aren't flagged as started--which makes sense--meaning the data disappears when rebuilding the container. Therefore #10 won't work.

I changed my patch to use a less convoluted approach to overriding the Messenger service. Dunno why I did it that way originally. So basically this is just a fix up to #21. Better tests still needed. And any feedback would be much appreciated.

kim.pepper’s picture

I'm not sure this is an issue with the Messenger service directly, or a more general one with any service after a container rebuild.

In the referring issue #474684: Allow themes to declare dependencies on modules the solution seems to require a number of services be refreshed after container rebuild:

<?php
/**
  * Refresh services after container rebuild.
  */
protected function refreshServices() {
    $container = $this->kernel->getContainer();
    $this->messenger = $container->get('messenger');
    $this->themeHandler = $container->get('theme_handler');
    $this->configFactory = $container->get('config.factory');
}

?>
kalpaitch’s picture

@kim.pepper I think this might be an 'if we're refreshing services let's do all of them' rather than any guaranteed issue with the other two. The messenger service specifically loses the references to the session.

alexpott’s picture

So this almost fixes another very important issue but unfortunately it doesn't quite because with this patch if you put a drupal_set_message() into code that occurs before the request is put the request is placed on the request stack in prehandle() you'll see

The website encountered an unexpected error. Please try again later.
TypeError: Argument 1 passed to Drupal\Core\Session\SessionConfiguration::getOptions() must be an instance of Symfony\Component\HttpFoundation\Request, null given, called in /Volumes/devdisk/dev/sites/drupal8alt.dev/core/lib/Drupal/Core/Session/SessionManager.php on line 111 in Drupal\Core\Session\SessionConfiguration->getOptions() (line 40 of core/lib/Drupal/Core/Session/SessionConfiguration.php).
Drupal\Core\Session\SessionConfiguration->getOptions(NULL) (Line: 111)
Drupal\Core\Session\SessionManager->start() (Line: 302)
Symfony\Component\HttpFoundation\Session\Storage\NativeSessionStorage->getBag('flashes') (Line: 249)
Symfony\Component\HttpFoundation\Session\Session->getBag('flashes') (Line: 259)
Symfony\Component\HttpFoundation\Session\Session->getFlashBag() (Line: 49)
Drupal\Core\Messenger\Messenger->getFlashBag() (Line: 69)
Drupal\Core\Messenger\Messenger->addMessage('herre', 'status', ) (Line: 50)
Drupal\Core\Messenger\LegacyMessenger->addMessage('herre', 'status', ) (Line: 489)
drupal_set_message('herre') (Line: 565)
Drupal\Core\DrupalKernel->preHandle(Object) (Line: 45)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 99)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1) (Line: 78)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 50)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 23)
Stack\StackedHttpKernel->handle(Object, 1, 1) (Line: 665)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)

If you put it after you'll see the message - which is an improvement on HEAD but now we're more brittle.

alexpott’s picture

Also #29 is definitely an issue with the new messanger service. The old service worked for super early messaging and now we've lost that.

alexpott’s picture

Here's a patch that doesn't have a different class for the early installer it just swaps session storage out for something that'll work. And it also fixes creating messages before the sessions is started and before the request is pushed to the stack.

Status: Needs review » Needs work

The last submitted patch, 31: 2940148-31.patch, failed testing. View results

alexpott’s picture

Status: Needs work » Needs review
FileSize
1.55 KB
5.9 KB

This should fix some of the errors.

alexpott’s picture

Issue summary: View changes

Updated issue summary.

Status: Needs review » Needs work

The last submitted patch, 33: 2940148-33.patch, failed testing. View results

alexpott’s picture

Status: Needs work » Needs review
FileSize
1.79 KB
6.38 KB

Another go.

kim.pepper’s picture

Thanks for taking the lead on this Alex.

Just a couple of small nitpicks I noticed.

  1. +++ b/core/lib/Drupal/Core/Messenger/Messenger.php
    @@ -27,6 +27,11 @@
    +  protected $messageSet = FALSE;
    

    Missing comment.

  2. +++ b/core/lib/Drupal/Core/Messenger/Messenger.php
    @@ -127,8 +133,21 @@
    +   *   TRUE if there are no messages
    

    Nit. Full stop.

jibran’s picture

+++ b/core/core.services.yml
@@ -1672,7 +1672,7 @@ services:
-    arguments: ['@session.flash_bag', '@page_cache_kill_switch']
+    arguments: ['@session', '@page_cache_kill_switch']

Is this change not a BC break?

kim.pepper’s picture

Not according to the policy:

https://www.drupal.org/core/d8-bc-policy#constructors

Constructors for service objects, plugins, and controllers
The constructor for a service object (one in the container), plugin, or controller is considered internal unless otherwise marked, and may change in a minor release. These are objects where developers are not expected to call the constructor directly in the first place. Constructors for value objects where a developer will be calling the constructor directly are excluded from this statement.

alexpott’s picture

Here's a test for the super early messaging... and addresses #37

alexpott’s picture

Title: Messenger service loses reference to flashbag reference on rebuild » Messenger service can't set messages super early and loses reference to flashbag reference on rebuild

The last submitted patch, 40: 2940148-40.test-only.patch, failed testing. View results

alexpott’s picture

Here's a test for setting a message before a module is installed and the container rebuilt.

alexpott’s picture

So the test in #43 proves nothing. I've tried making DrupalSetMessageTest

class DrupalSetMessageTest extends KernelTestBase {

  /**
   * {@inheritdoc}
   */
  protected static $modules = ['system'];

  /**
   * The basic functionality of drupal_set_message().
   */
  public function testDrupalSetMessage() {
    drupal_set_message(t('A message: @foo', ['@foo' => 'bar']));
    $messages = drupal_get_messages();
    $this->assertInstanceOf('Drupal\Core\Render\Markup', $messages['status'][0]);
    $this->assertEquals('A message: bar', (string) $messages['status'][0]);

    // Test that installing a module does not lose a message.
    \Drupal::messenger()->addStatus('A test');
    $this->container->get('module_installer')->install(['system_test']);
    $messages = \Drupal::messenger()->all();

    $this->assertEquals(['A test'], (string) $messages['status'][0]);
  }

}

test it but this proves that the current patch doesn't work in kernel tests. It may well work when not in that environment when there is a real session - but this also opens questions about differences between CLI and browser. I could get the test to pass if I add:

    tags:
      - { name: persist }

to both the session_manager and messenger services. But that feels very wrong.

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.

angelamnr’s picture

Rerolled patch in #43 for 8.8.x

Status: Needs review » Needs work

The last submitted patch, 48: 2940148-48.patch, failed testing. View results

alexpott’s picture

Status: Needs work » Needs review
FileSize
1.73 KB
12.4 KB

Interesting fail. Patch attached fixes it. We're saving the session too early to use Symfony flash based messages. I think it's working in head because it is relying on the legacy messenger.

I think this patch either fix some other problems we've had with session in the installer OR maybe will fail because of new things :) fun.

Also #48 removed the changes to core/modules/system/tests/src/Functional/Bootstrap/DrupalSetMessageTest.php as that test no longer exists because it no longer exists so we need to try to add that back.

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.

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

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

jjpilcher’s picture

Rerolled patch in #50 for 9.1.x. First time doing this so any feedback is appreciated.

nikitagupta’s picture

Rerolled patch #55, also resolved the CI error.

@jjpilcher, the latest version of this issue is 9.3.x-dev so we have to reroll the patch on the 9.3.x-dev version.

Status: Needs review » Needs work

The last submitted patch, 56: 2940148-56.patch, failed testing. View results

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

mstrelan’s picture

I came across this issue when looking at #3189457: Status message does not appear after running /core/rebuild.php. Re-rolled #56, but left out changes to system_test_module_preinstall since it looks like we already set a message there and test for it.

Abhisheksingh27’s picture

Fixed ccf issue of #61 patch

Status: Needs review » Needs work

The last submitted patch, 62: 2940148-62.patch, failed testing. View results

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.