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
Comment | File | Size | Author |
---|---|---|---|
#62 | interdiff_61_62.txt | 4.98 KB | Abhisheksingh27 |
#62 | 2940148-62.patch | 12.66 KB | Abhisheksingh27 |
#61 | 2940148-61.patch | 10.51 KB | mstrelan |
#56 | 2940148-56.patch | 11.62 KB | nikitagupta |
#55 | 2940148_91x.patch | 12.66 KB | jjpilcher |
Comments
Comment #2
kalpaitch CreditAttribution: kalpaitch as a volunteer and commentedComment #3
larowlanThis is arguably a form of data loss
Comment #4
dillix CreditAttribution: dillix commented@larowlan should it be backported to 8.5, or we can live without it till 8.6?
Comment #5
larowlanBugs are always eligible for backport
Comment #6
markhalliwellWe 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?Comment #7
kevineinarsson CreditAttribution: kevineinarsson as a volunteer commentedPatch 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.
Comment #8
kevineinarsson CreditAttribution: kevineinarsson as a volunteer commentedComment #9
markhalliwellComment #10
kevineinarsson CreditAttribution: kevineinarsson as a volunteer commentedNeeds a bootstrapped browser test / a better test because this doesn't make sure that old messages are being carried over.
Comment #11
dawehnerI'm not convinced of this solution: #2368263: Remove the persist service tag makes this pretty clear.
This solution might help ...
Comment #13
kevineinarsson CreditAttribution: kevineinarsson as a volunteer commentedAh, okay. Didn't know about that. My patch had a typo anyways -- let's just hide that.
Comment #14
kevineinarsson CreditAttribution: kevineinarsson as a volunteer commentedUsing the approach in #11 but using the Session service instead of the Session Manager service works... Still no tests.
Comment #15
kevineinarsson CreditAttribution: kevineinarsson as a volunteer commentedComment #17
dawehner@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?
Comment #18
kevineinarsson CreditAttribution: kevineinarsson commentedThe 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.
Comment #19
kevineinarsson CreditAttribution: kevineinarsson commentedDoes something like this make sense? Should fix the service trying to spin up the database before db setup.
Comment #21
kevineinarsson CreditAttribution: kevineinarsson commentedRight, 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..?
Comment #23
markhalliwellThe 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.Comment #24
kevineinarsson CreditAttribution: kevineinarsson commented@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
orsession_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 theLegacyMessenger
, we could catch theDatabaseNotFoundException
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 themessenger
service seems to be the way to go.Comment #25
kevineinarsson CreditAttribution: kevineinarsson as a volunteer commented@dawehner, @markcarver: Thoughts?
Comment #26
kevineinarsson CreditAttribution: kevineinarsson as a volunteer commentedFinally 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.
Comment #27
kim.pepperI'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:
Comment #28
kalpaitch CreditAttribution: kalpaitch as a volunteer and commented@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.
Comment #29
alexpottSo 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
If you put it after you'll see the message - which is an improvement on HEAD but now we're more brittle.
Comment #30
alexpottAlso #29 is definitely an issue with the new messanger service. The old service worked for super early messaging and now we've lost that.
Comment #31
alexpottHere'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.
Comment #33
alexpottThis should fix some of the errors.
Comment #34
alexpottUpdated issue summary.
Comment #36
alexpottAnother go.
Comment #37
kim.pepperThanks for taking the lead on this Alex.
Just a couple of small nitpicks I noticed.
Missing comment.
Nit. Full stop.
Comment #38
jibranIs this change not a BC break?
Comment #39
kim.pepperNot according to the policy:
https://www.drupal.org/core/d8-bc-policy#constructors
Comment #40
alexpottHere's a test for the super early messaging... and addresses #37
Comment #41
alexpottComment #43
alexpottHere's a test for setting a message before a module is installed and the container rebuilt.
Comment #44
andypostComment #45
alexpottSo the test in #43 proves nothing. I've tried making DrupalSetMessageTest
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:
to both the session_manager and messenger services. But that feels very wrong.
Comment #48
angelamnr CreditAttribution: angelamnr commentedRerolled patch in #43 for 8.8.x
Comment #50
alexpottInteresting 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.
Comment #55
jjpilcher CreditAttribution: jjpilcher as a volunteer commentedRerolled patch in #50 for 9.1.x. First time doing this so any feedback is appreciated.
Comment #56
nikitagupta CreditAttribution: nikitagupta at Srijan | A Material+ Company for Drupal India Association commentedRerolled 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.
Comment #61
mstrelan CreditAttribution: mstrelan at PreviousNext commentedI 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.Comment #62
Abhisheksingh27 CreditAttribution: Abhisheksingh27 at OpenSense Labs for DrupalFit commentedFixed ccf issue of #61 patch