Add \Drupal\Core\Messenger\Messenger

Unfortunately I got zero occurrences in code search if i search for "Messanger" or "SessionMessenger" .

First try.

I tried to fix rest of the fails but no luck. Any suggestion @dawehner? I think the issue is because session service and messenger service both are using the same flashbag. Maybe, we should change the name of the messenger flashbag.

RE: #13

1. Yeah.
2. Well this class is unit test able so let's add unit test instead and `unset($_SESSION['messages']['status'][0]);` was a hack imo. Do we want to keep on supporting this? We can't do this anymore with current implementation unless we provide a wrapper around \Symfony\Component\HttpFoundation\Session\Flash\FlashBag::setAll or \Symfony\Component\HttpFoundation\Session\Flash\FlashBag::set. I think we should

Time to summon @znerol!

Let's restore the kill-switch for the moment.

WoW it passed.

I'm looking closer at the D\s\T\System\FrontPageTest now. First looking through all verbose output to check for printed messages. I see the following:

FrontPageTest.php:56

info: On front page.

FrontPageTest.php:60

info: On front page.

FrontPageTest.php:67

error: The path '/kittens' is either invalid or you do not have access to it.

FrontPageTest.php:72

error: The path 'node/1' has to start with a slash.

FrontPageTest.php:77

info: The configuration options have been saved.

FrontPageTest.php:80

info: On front page.

FrontPageTest.php:84

info: On front page.

Set a breakpoint inside D\C\P\ResponsePolicy\KillSwitch::trigger() and confirm that inside the test nothing else than those messages invoke the page cache kill switch.

Previous test results and observations of @dawehner indicate that without the kill switch pages get cached even though a message is printed on the page. This indicates that we currently do not cancel page caching when printing messages. So maybe we can fix that and then try to eliminate the kill switch again.

D\s\Plugin\Block\SystemMessagesBlock:getCacheMaxAge returns Cache::PERMANENT, the comment says that messages get loaded lazily. Maybe, FrontPageTest shouldn't rely on messages to assert whether or not the front page is displayed? Unless there is a way to assert on lazy loaded content maybe.

Unassigning myself, ideas?

Maybe we should tackle the elimination of the kill switch in another issue?

First of all, thank you @znerol for jumping in and making this green again your help is much appreciated.

> Maybe we should tackle the elimination of the kill switch in another issue?

Right now LegacyMessenger is purely internal. Not elimination the kill switch means we can't make Messenger part of public API not until we provide the BC in follow up. Personally, I'd like to kill the kill switch in this issue.

Let's ping @Wim Leers about caching. I think we need to implement cache tag to get around Cache::PERMANENT. Some kind of method on Messanger service to add the hash of flashbag to the cache tags.

We need a replacement response policy which is able to detect whether it is safe to cache a page. I'm not quite certain about the logic needed here.

We cannot rely on the contents of the session because that will be already empty when the response policy is invoked.

Depending on whether the lazy-loader decides to include the message fragment directly or rather output a placeholder, we need to vary the behavior of the response policy. In the former case, the page is cachable, in the latter it is not.

Do we need to disable the page cache if any cache contexts bubble up to the page?

Do we need to disable the page cache if any cache contexts bubble up to the page?

This is impossible because we always seem to have some cache contexts bubbling all the way up.

Another interesting observation: Set a breakpoint at line 26 in KillSwitch:check (the line which says return static::Deny;). When the debugger hits that breakpoint while running the FrontPageTest the stack trace indicates that the response policy was invoked from the Dynamic Page Cache.

Interesting: D/C/Render/Element/StatusMessages::renderMessages() does not specify any cache metadata. Even though it is lazy-loaded, it is still supposed to return the session cache context, isn't it? Maybe even unconditionally (if there is no message), since you never know whether there is one on the next request?

If the above is true, then it might be beneficial for the messanger service to implement the CacheableDependencyInterface such that the StatusMessages element simply can forward the cache metadata.

Guess that's a question for @Wim Leers.

1. +++ b/core/lib/Drupal/Core/Messenger/MessengerInterface.php
   @@ -123,6 +127,9 @@ public function deleteAll();
   +   *   The deleted messages of given type..
   

   Nit: s/.././

2. +++ b/core/modules/system/tests/modules/system_test/src/Controller/SystemTestController.php
   @@ -96,13 +96,6 @@ public function mainContentFallback() {
   -    // Set two messages.
   -    drupal_set_message('First message (removed).');
   -    drupal_set_message(t('Second message with <em>markup!</em> (not removed).'));
   

   Why remove these but not everything below? IOW: +1 to the concern in #13.2.

I'll try to answer the things that I was pinged for here. But first of all: the issue scope ("let's kill the kill switch") does NOT match what the issue title & summary say.

The killswitch is fundamental to how storing/processing/rendering of status messages works in HEAD (as well as in D7). If you want to change that, then yes, there are going be consequences elsewhere.

In HEAD:

1. Any call to drupal_set_message() while running the code that is building a response to the current request would cause PageCache (= for anon users) to not cache the response. But also Dynamic Page Cache (= authenticated users) respects the killswitch:
   

     page_cache_kill_switch:
       class: Drupal\Core\PageCache\ResponsePolicy\KillSwitch
       tags:
         - { name: page_cache_response_policy }
         - { name: dynamic_page_cache_response_policy }
   

2. \Drupal\system\Plugin\Block\SystemMessagesBlock simply renders ['#type' => 'status_messages'] and the block itself says it's endlessly cacheable, and it even includes the explanation in its comment:
   

     public function getCacheMaxAge() {
       // The messages are session-specific and hence aren't cacheable, but the
       // block itself *is* cacheable because it uses a #lazy_builder callback and
       // hence the block has a globally cacheable render array.
       return Cache::PERMANENT;
     }
   

3. If we then inspect \Drupal\Core\Render\Element\StatusMessages (which corresponds to ['#type' => 'status_messages']), we see that it generates a placeholder unconditionally, i.e. 100% of the time. This means it's perfectly cacheable by Render Cache & Dynamic Page Cache: because there is no variation.
4. Finally, \Drupal\Core\Render\Element\StatusMessages::renderMessages() is the #lazy_builder callback that runs once on every page. It doesn't return cacheability metadata because it doesn't matter anymore at this point: the #cache key on the render array it returns is not specified because we don't ever need/want rendered status messages to be render cached anyway. And this lazy builder callback is invoked for something that is placeholdered 100% of the time (see previous point.)

Apparently we want to remove the kill switch. Okay, that'd be lovely to remove indeed. So that's what we want to change. What remains the same? We still want a placeholder 100% of the time, we just don't know if it'll get some content or not ahead of time. What matters, is whether a message was set during this request or not.
It should be possible to use a cache context to communicate this. In HEAD, we should be able to remove the call to the kill switch and instead always let the lazy builder callback return the session cache context (just like @znerol suggested). This will cause Page Cache to not cache the page, thanks to \Drupal\Core\PageCache\RequestPolicy\NoSessionOpen.

However, if that's where we stop, then the test still fails. Why? Because only Page Cache respects \Drupal\Core\PageCache\RequestPolicy\NoSessionOpen. Dynamic Page Cache does not. So we have to dig a bit deeper: in HEAD, we 1) render the overall page, 2) which included a call to drupal_set_message() and therefore to the killswitch, then 3) let Dynamic Page Cache store the result, then 4) render the placeholders, and finally 5) let Page Cache store the result.
In HEAD, the killswitch is invoked at step 2, and hence it prevents both Page Cache and Dynamic Page Cache from caching the response. With this patch, Dynamic Page Cache is not yet being informed that it shouldn't be caching the response. Because it's only in step 4) that the messages placeholder is rendered, and hence the session cache context is returned there — after Dynamic Page Cache has already stored the result.
Therefore system_test_page_attachments() doesn't run again — upon the next visit. Only lazy builders for placeholders on the page are invoked again. Therefore the drupal_set_message(t('On front page.')); is not invoked again.

Now, honestly, system_test_page_attachments() is an abuse of what that hook is designed to do. It's a hook designed for page-level render array attachments, not for sending messages!

So, really, I think that the test is at fault here. Adding $page['#cache']['max-age'] = 0; fixes it.

I think we need to implement cache tag to get around Cache::PERMANENT

Using cache tags for this doesn't make sense, because no identifiable data is being stored across requests that may change in the future and hence need invalidation.

Thanks, @Wim Leers. I have to read it 4 times to understand it.

The issue started as removing $_SESSION and then I removed kill switch in new implementation and we all went down 'remove kill switch' rabbit hole.
If #26 patch will pass FrontPageTest then I'm going to create a new issue to remove the kill switch, move patch from #26 to new issue, and postpone this one on the new issue. LegacyMessenger is internal so we can play around with API before making it public here.

Now, honestly, system_test_page_attachments() is an abuse of what that hook is designed to do. It's a hook designed for page-level render array attachments, not for sending messages!

Instead of using drupal_set_message in system_test_page_attachments() why not add the lazy builder for frontpage in the function and use drupal_set_message there. The main goal of \Drupal\system\Tests\System\FrontPageTest is to test the front page functionality which we can achive like this.

It is clearly stated in hook_page_attachments documentation that Add attachments (typically assets) to a page before it is rendered. so I don't think removing the kill switch will introduce the regression as shown by FrontPageTest.

Created #2908026: Remove killswitch from LegacyMessenger and uploaded #26 there. Removing the tags as out of scope changes are moved to #2908026.

So, really, I think that the test is at fault here. Adding $page['#cache']['max-age'] = 0; fixes it.

Oh, right! This is actually quite obvious. In fact it would be useful to formulate this as a generic rule (also for the change-notice because contrib test cases might fail as well due to this change). Maybe something like this:

Messages should only be emitted in response to an action, i.e. on an uncacheable request. For example when a node is submitted or a comment was changed. If a messages is emitted on a cacheable response, then it is completely unpredictable when this message is going to be displayed to the user. This is due to the fact that the content might be served from the cache and hence the code emitting the message is only invoked during a cache MISS.

Any help in making the tests green in #2908026: Remove killswitch from LegacyMessenger would be appreciated.

I had a discussion with @Wim Leers about this issue at DC Vienna. He purposed that instead of injecting kill switch we could use the service directly from the container that would not block us on #2908026: Remove killswitch from LegacyMessenger and we wouldn't have to make any changes to public API (\Drupal\Core\Messenger\Messenger) once we would be able to remove kill switch. Also, he thinks fixing #2908026: Remove killswitch from LegacyMessenger is a good idea.
Here is the updated patch. The interdiff is against #11. #12 is still to be addressed.

+++ b/core/lib/Drupal/Core/Messenger/Messenger.php
@@ -0,0 +1,102 @@
+    return $this->flashBag->get($type);

Jibran and I discussed this issue at DrupalSouth and agreed to add a comment to explain that 'get' from a flashbag actually deletes the message.

Added the docs.

LGTM

+++ b/core/modules/system/tests/src/Functional/Bootstrap/DrupalSetMessageTest.php
@@ -22,11 +22,7 @@ class DrupalSetMessageTest extends BrowserTestBase {
-    // The page at system-test/drupal-set-message sets two messages and then
-    // removes the first before it is displayed.
     $this->drupalGet('system-test/drupal-set-message');
-    $this->assertNoText('First message (removed).');
-    $this->assertRaw(t('Second message with <em>markup!</em> (not removed).'));

Discussed with @kim.pepper and @jibran at Drupal South- let's leave this test in, but change the implementation. I don't think we want the ability to unset as part of the public API (i.e. we don't want to add methods to the MessagerInterface) but we should retain the test to prove that it is still possible. We can do the removal by calling deleteByType which will return the array with two entries, then unset one and re add the other.

Thanks, for the review. Addressed #36.

+++ b/core/modules/system/tests/modules/system_test/src/Controller/SystemTestController.php
@@ -96,6 +108,17 @@ public function mainContentFallback() {
+    $messages = $this->messenger->deleteByType('status');

I tried not to use deprecated drupal_get_messages('status') and I tried to keep changes to the minimum because after injecting messenger service we can replace all drupal_set_message but we can do that leter.

Back to RTBC

Thanks for working on this.

I almost NWed this for a BC break (even an internal one), until I blamed and saw it was only added in 8.5.x.

Updating the summary to explain why this is allowed change up until alpha1, and to indicate that it must deprecate rather than removing thereafter.

I tried not to use deprecated drupal_get_messages('status')

Just a small note that these lagecy methods are not deprecated here and after #2278383: Create an injectible service for drupal_set_message() we didn't create a followup for that so I created one #2924538: [META] Remove all usages of drupal_set_message and drupal_get_messages.

Edit: I blame sleep deprivation for this reply :). We should definitely add deprecation error here as @xjm explained in the next comment. Sorry about the noise.

Thanks @jibran.

@larowlan and I discussed this issue briefly. Since we're deprecating drupal_set_message() and drupal_get_messages(), we should also add the @trigger_error() calls within their code paths. @larowlan mentioned that we'd likely add a lot of test failures for that, but we should be able to add the messages to the list of ignored deprecated messages in getSkippedDeprecations(), and file a followup issue to re-remove it and fix the many fails that'd result.

Added the trigger errors. Discussed with @larowlan at DrupalSouth and split out the DeprecationListener changes so we can see if the trigger_error stuff fails the builds.

@larowlan pointed out the pass patch needs to be last, so re-uploading.

+++ b/core/includes/bootstrap.inc
@@ -455,8 +455,13 @@ function watchdog_exception($type, Exception $exception, $message = NULL, $varia
+  @trigger_error("drupal_set_message() is deprecated Drupal 8.5.0, will be removed before Drupal 9.0.0. Use \Drupal::service('messenger')->addMessage() instead. See https://www.drupal.org/node/2774931", E_USER_DEPRECATED);

@@ -487,8 +492,14 @@ function drupal_set_message($message = NULL, $type = 'status', $repeat = FALSE)
+  @trigger_error("drupal_get_message() is deprecated Drupal 8.5.0, will be removed before Drupal 9.0.0. Use \Drupal::service('messenger')->addMessage() instead. See https://www.drupal.org/node/2774931", E_USER_DEPRECATED);

+++ b/core/tests/Drupal/Tests/Listeners/DeprecationListener.php
@@ -111,6 +111,8 @@ public static function getSkippedDeprecations() {
+      'drupal_set_message() is deprecated Drupal 8.5.0, will be removed before Drupal 9.0.0. Use \Drupal::service(\'messenger\')->addMessage() instead. See https://www.drupal.org/node/2774931',
+      'drupal_get_message() is deprecated Drupal 8.5.0, will be removed before Drupal 9.0.0. Use \Drupal::service(\'messenger\')->addMessage() instead. See https://www.drupal.org/node/2774931',

Both need to use " and ', or ' and \'. But they need to match exactly!

Ah thanks Tim!

Hmm. Pretty sure the message matches now.

Yep, it does match. And I ran this locally and put a breakpoint in \Drupal\Tests\Listeners\DeprecationListener::endTest() where the unset() is.

No idea what's happening here, unless getenv('SYMFONY_DEPRECATIONS_SERIALIZE') is false-y.

Discussed at DrupalSouth with @larowlan about whether we should reference the method on the interface instead of Drupal::service(). If the issue is with quotes, then at least we can also rule that out.

Also fixed incorrect method names in the deprecation annotation for drupal_get_message().

+++ b/core/includes/bootstrap.inc
@@ -487,8 +492,14 @@ function drupal_set_message($message = NULL, $type = 'status', $repeat = FALSE)
+ *   Use \Drupal::service('messenger')>all() or
+ *   \Drupal::service('messenger')>messagesByType() instead.

Should this be changed to use the interface as well? (also typo with > instead of ->)

Discussed with @larowlan in slack and agreed to update annotations to match the trigger_error message, ie. reference the methods on the interface.

Removing the needs followup tag as #2924538: [META] Remove all usages of drupal_set_message and drupal_get_messages is turned into that followup. Everything mentioned in #41 is addressed so RTBC.

Thanks, @xjm, @kim.pepper, @tim.plunkett and @larowlan!

--- lib/Drupal/Core/Messenger/MessengerInterface.php
+++ PHP_CodeSniffer
@@ -117,7 +117,7 @@
    * Deletes all messages.
    *
    * @return string[]|\Drupal\Component\Render\MarkupInterface[]
-   *    The deleted messages.
+   *   The deleted messages.
    */
   public function deleteAll();
 

PHPCS error.

Fixes #58

+++ b/core/includes/bootstrap.inc
@@ -487,8 +492,14 @@ function drupal_set_message($message = NULL, $type = 'status', $repeat = FALSE)
   /** @var \Drupal\Core\Messenger\MessengerInterface $messenger */
   $messenger = \Drupal::hasService('messenger') ? \Drupal::service('messenger') : NULL;
   if ($messenger && ($messages = $messenger->all())) {

One tiny trivial request:

Considering that getting/setting messages can and will still likely be used in a lot of procedural code, can we add a static \Drupal::messenger() helper here that does the same thing as this code? This is really no different than what was done for \Drupal::logger() and will help reduce the need for "hasService" and type hints elsewhere.

Added \Drupal::messenger()

Realised I didn't address the 'hasService' check comment in #61.

Since we will always have the service now, do we actually need this? Is Drupal::messenger() enough?

I updated the change record to refer to \Drupal::messenger() in the examples.

Since we will always have the service now, do we actually need this?

I'm not entirely sure. This logic is only in drupal_get_message() and from what I can deduce of #2278383-80: Create an injectible service for drupal_set_message(), this was only ever added as a workaround for Drush.

Based on the above comment, it was removed from drupal_set_message(), but not drupal_get_message(). Perhaps it was simply overlooked, or perhaps it's still needed and is specific to only drupal_get_message() (a proceedural function used by Drush).

It's definitely not needed in \Drupal::messenger() though, so current implementation here is fine.

---

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

+++ b/core/lib/Drupal/Core/Messenger/Messenger.php
@@ -0,0 +1,103 @@
+    \Drupal::service('page_cache_kill_switch')->trigger();

One more tiny nit: until #2908026: Remove killswitch from LegacyMessenger actual lands (which will likely be after this), the injected page_cache_kill_switch service should still be kept.

One more tiny nit: until #2908026: Remove killswitch from LegacyMessenger actual lands (which will likely be after this), the injected page_cache_kill_switch service should still be kept.

This was discussed already in #32

Based on the above comment, it was removed from drupal_set_message(), but not drupal_get_message(). Perhaps it was simply overlooked, or perhaps it's still needed and is specific to only drupal_get_message() (a proceedural function used by Drush).

I manually tested successfully using this patch using drush site-install.

Thanks! @kim.pepper for answering #67 back to RTBC.

This was discussed already in #32

Yes, I am aware of that. I failed to explain why. Here goes:

This use of a, currently, vital service in such a potentially high traffic method (addMessage) is quite simply wrong.

The primary reasoning behind doing this was centered around the idea of "we wouldn't have to make any changes to public API".

IMO, this is a very weak argument and kind of defeats the whole point of using services in the first place.

MessengerInterface is actually the public facing API, not Messenger.

MessengerInterface does not contain a defined __construct() method (like all other services) for this exact reason: to utilize new or different services should the need arise.

The reality is that, yes, we're hoping that this kill switch will also be removed before 8.5.0.

If it does, then great, we're not breaking any BC code because this entire API was created in 8.5.x.

If it's not, then we will have to ship with the kill switch and simply deal with that reality.

C'est la vie. We shouldn't sacrifice proper code on the hopes and dreams that something "may land" in an entirely separate issue.

edit: Even that wouldn't be a BC break though because we wouldn't be removing anything from MessengerInterface, so we'd be fine.

I manually tested successfully using this patch using drush site-install.

I imagine that is because drupal_get_message() is still using the previously introduced ternary \Drupal::hasService() condition, not the new \Drupal::messenger().

Above discussion makes me wonder if we should profile this?

Unrelated nitpick: Can we please remove all the comma splices? "Deprecated in 8.5.x, will be removed before 9.0.0" isn't quite grammatically correct in English. It can be made correct by replacing the comma with the word "and". :)

@markcarver, can you clarify what, specifically, you are suggesting to change here? We weren't quite sure what the issue was NW for. Thanks!

I imagine that is because drupal_get_message() is still using the previously introduced ternary \Drupal::hasService() condition, not the new \Drupal::messenger().

Yes, you are right. I removed the check, and it does fail.

Discussed with @larowlan who suggested an in-memory MemoryMessenger, which only stores messages in memory for early bootstrap when the service doesn't exist. Had a first go at this.

Also, happy to inject KillSwitch if there are no issues with BC breaks.

Fixes @xjm comma splices too.

Successfully manually tested drush site-install with the above patch.

Also, happy to inject KillSwitch if there are no issues with BC breaks.

Other than this change it looks ok to me. Can we add tests to replicate drush si fail?

Above discussion makes me wonder if we should profile this?
...
@markcarver, can you clarify what, specifically, you are suggesting to change here?

I was specifically referring to my review in #67 about prematurely removing the dependency injected kill switch service which is actually what #2908026: Remove killswitch from LegacyMessenger is for.

I don't think this actually needs to be profiled. Having a ton of messages added in a single page request isn't very common, but it theoretically could happen. An example, while a poor design for sure, is a batch operation affecting 1000s of objects that also adds a message for each item. I doubt that would ever happen in core.

Regardless, it was less about performance and more so just ensuring that we use existing and proper coding paradigms: services that depend on other services are injected; they don't call \Drupal::service() statically.

---

Almost there :D

1. +++ b/core/includes/bootstrap.inc
   @@ -455,8 +455,13 @@ function watchdog_exception($type, Exception $exception, $message = NULL, $varia
      /* @var \Drupal\Core\Messenger\MessengerInterface $messenger */
      $messenger = \Drupal::service('messenger');
   

   drupal_set_message() should also use \Drupal::messenger() now as well.

2. +++ b/core/lib/Drupal.php
   @@ -757,4 +758,17 @@ public static function messenger() {
   +    if (static::hasService('messenger')) {
   +      return static::getContainer()->get('messenger');
   +    }
   +    return new MemoryMessenger();
   

   This gets a little tricky here I think.

   On one hand, this is creating a new MemoryMessenger each and every time this method is invoked, thus eating any existing object. It needs to be statically saved.

   On the other hand, because this could potentially be called before the container is available and is set using MemoryMessenger, what happens if the container suddenly becomes available after it has been properly initialized in the bootstrap process? The existing messages in memory are lost. I think these messages need to be merged into the service if there's an existing MemoryMessenger object.

   This is a rough draft code rewrite (written here):

   static $messenger;
   
   // Use the real Messenger service if the container has been fully initialized.
   if (static::hasService('messenger')) {
     $existing = $messenger;
     $messenger = static::getContainer()->get('messenger');
   
     // Merge any existing messages in memory to the initialized service.
     if ($existing instanceof MemoryMessenger) {
       foreach ($existing->all() as $type => $messages) {
         foreach ($messages as $message) {
           $messenger->addMessage($message, $type);
         }
       }    
     }
   }
   
   // Create a fallback MemoryMessenger object if no service exists.
   if (!isset($messenger)) {
     $messenger = new MemoryMessenger();
   }
   
   return $messenger;
   

---

Can we add tests to replicate drush si fail?

AFAIK, core's policy is not to officially support 3rd party tools like Drush. I don't think there needs to be specific tests around drush specifically, but yes I think we could test using \Drupal::messenger() and adding messages before and after the container has been initialized to ensure that both can be retrieved at the end.

Instead of introducing new Memory messenger how about we do this. Interdiff is from #63.

No, this still doesn’t address the problem of losing messages when the container finally initializes. The problem isn’t MemoryMessenger, but rather how to handle the static method when it’s invoked before th container is initialized.

Yeah, I ignored the case in #79 when container is not available. Can we please move this discussion in the follow and not deprecate drupal_get_message here. This whole discussion is correct but seems out of scope.

No, this still doesn’t address the problem of losing messages when the container finally initializes. The problem isn’t MemoryMessenger, but rather how to handle the static method when it’s invoked before th container is initialized.

I assume we are just dropping these messages in head now?

Discussed with @larowlan (I need to stop saying that) and came up with the idea of a ChainedMessenger which would abstract the logic out of \Drupal::messenger().

The chained messenger would basically have 2 messengers, and have a method with the following logic:

<?php
  /**
   * Get the messenger to use.
   *
   * @return \Drupal\Core\Messenger\MessengerInterface
   *   The messenger.
   */
  protected function getMessenger() {
    // Shortcut if we have a messenger service.
    if (isset($this->serviceMessenger)) {
      return $this->serviceMessenger;
    }

    // If the service exists, but we haven't set it yet.
    if (\Drupal::hasService('messenger')) {
      $this->serviceMessenger = \Drupal::service('messenger');
      $this->mergeMessages($this->memoryMessenger, $this->serviceMessenger);
      return $this->serviceMessenger;
    }

    // Fallback to memory messenger.
    return $this->memoryMessenger;
  }
?>

I assume we are just dropping these messages in head now?

Technically, yes. But this is likely only happening for 3rd party services that are calling the procedural drupal_get_messages() and drupal_set_message() functions before actually instantiated the container (i.e. drush). Core itself is most likely "fine" since it relies on a fully instantiated container.

drupal_get_messages() does absolutely nothing if there is no service and drupal_set_message() would actually throw an error if there is no service just by attempting to retrieve it or at the very least when attempting to invoke a method on NULL.

came up with the idea of a ChainedMessenger

Cool :D Sounds like a good idea given the complexity.

Ultimately, MemoryMessenger and ChainedMessenger both are just boilerplate code for not breaking BC. In the future, we won't need either because the drupal_get_messages() and drupal_set_message() procedural functions will have been removed and the code using them will be forced to properly instantiate the container to use the normal Messenger service.

OK here goes.

Added a ChainedMessenger as per above.

I also abstracted out a few methods to an abstract BaseMessenger class.

Again, manually tested locally with drush site-install.

1. +++ b/core/lib/Drupal/Core/Messenger/BaseMessenger.php
   @@ -0,0 +1,50 @@
   +abstract class BaseMessenger implements MessengerInterface {
   

   We should mark this as @internal

2. +++ b/core/lib/Drupal/Core/Messenger/ChainedMessenger.php
   @@ -0,0 +1,109 @@
   + * This implementation uses a memory messenger for handling messages before the
   + * container is initialized. When initialised, it will copy over messages and
   + * use that thereafter.
   + */
   

   We should mark this as @internal

3. +++ b/core/lib/Drupal/Core/Messenger/MemoryMessenger.php
   @@ -10,7 +10,7 @@
   +class MemoryMessenger extends BaseMessenger {
   

   Same here

Fixes #87

Thanks, @larowlan and @kim.pepper for coming up with the new solution. The changes look good just some minor observations.

1. +++ b/core/lib/Drupal/Core/Messenger/ChainedMessenger.php
   @@ -0,0 +1,111 @@
   + * This implementation uses a memory messenger for handling messages before the
   + * container is initialized. When initialised, it will copy over messages and
   + * use that thereafter.
   

   I think we should also mention to use messenger service for all the code run after bootstrap and never use this service.

2. +++ b/core/lib/Drupal/Core/Messenger/ChainedMessenger.php
   @@ -0,0 +1,111 @@
   +  protected $serviceMessenger;
   

   Let's call it $messengerService.

3. +++ b/core/lib/Drupal/Core/Messenger/Messenger.php
   @@ -0,0 +1,90 @@
   +    $this->killSwitch->trigger();
   

   Can we uninject(eject is the right word I think) this as per #32?

Can we uninject(eject is the right word I think) this as per #32?

No. See #71/#78.

+++ b/core/includes/bootstrap.inc
 function drupal_set_message($message = NULL, $type = 'status', $repeat = FALSE) {
+  @trigger_error('drupal_set_message() is deprecated Drupal 8.5.0 and will be removed before Drupal 9.0.0. Use \Drupal\Core\Messenger\MessengerInterface::addMessage() instead. See https://www.drupal.org/node/2774931', E_USER_DEPRECATED);
   /* @var \Drupal\Core\Messenger\MessengerInterface $messenger */
   $messenger = \Drupal::service('messenger');
   $messenger->addMessage($message, $type, $repeat);

This needs to now be using \Drupal::messenger() as well.

Fixes #89 1. and 2.
Fixes #91

Also moved some more duplication into the base class.

Also moved some more duplication into the base class.

I think that broke things. It's only a few lines of code. Do we really need a doAddMessage method just for determining if a message should be added?

Re: #94 reverted those changes.

Re: #95

1. This is the only place the chained messenger gets initialised.
2. There is a comment above why we aren't using strict array checking.
3. Added a unit test

+++ b/core/includes/bootstrap.inc
@@ -455,10 +455,14 @@ function watchdog_exception($type, Exception $exception, $message = NULL, $varia
 function drupal_set_message($message = NULL, $type = 'status', $repeat = FALSE) {
-  /* @var \Drupal\Core\Messenger\MessengerInterface $messenger */
-  $messenger = \Drupal::service('messenger');
+  @trigger_error('drupal_set_message() is deprecated Drupal 8.5.0 and will be removed before Drupal 9.0.0. Use \Drupal\Core\Messenger\MessengerInterface::addMessage() instead. See https://www.drupal.org/node/2774931', E_USER_DEPRECATED);
+  $messenger = \Drupal::messenger();
   $messenger->addMessage($message, $type, $repeat);
   return $messenger->all();
 }

I suspect this change is causing the fails, but no idea why.

I suspect this change is causing the fails, but no idea why.

I think the problem is that these tests are ensuring that the status messages are visible. Could it be because of trigger_error()? I know the tests are ignoring these, but that doesn't mean the messages are actually being added does it? I know there's a @ preceding it to prevent execution halt, but I don't know how reliable that really is. Does the rest of the code in the function actually get executed?

Regardless, I don't think this issue should be deprecating any code until core actually stops using these procedural functions. This issue should really just be about adding a real service so core can actually use it.

If anything, I think the trigger_error() bit and the @deprecated comments should ultimately be added as part of #2924538: [META] Remove all usages of drupal_set_message and drupal_get_messages.

P.S. I think there should be a test added that actually checks that ChainedMessenger::getMessenger returns the proper class based on whether the container has been initialized or not. The test added for merging messages is manually constructing the classes, but ChainedMessenger::mergeMessages should really be internal and only happen as part of ChainedMessenger::getMessenger.

I think the problem is that these tests are ensuring that the status messages are visible. Could it be because of trigger_error()? I know the tests are ignoring these, but that doesn't mean the messages are actually being added does it? I know there's a @ preceding it to prevent execution halt, but I don't know how reliable that really is. Does the rest of the code in the function actually get executed?

Regardless, I don't think this issue should be deprecating any code until core actually stops using these procedural functions. This issue should really just be about adding a real service so core can actually use it.

@xjm asked for this in #39 and #41.

I'm well aware that it was asked for, but that is only because @deprecated was already added to the DOXYGEN block.

I'm just saying that we shouldn't deprecate anything at all in this issue right now.

It's rare and generally bad practice to deprecate something that is still technically being used internally.

Most deprecations happen when core switches to the new implementation and no longer uses deprecated code, i.e. what will happen in #2924538: [META] Remove all usages of drupal_set_message and drupal_get_messages.

@mark.carver - @xjm asked for the deprecation and trigger_error here

+++ b/core/lib/Drupal/Core/Messenger/Messenger.php
@@ -0,0 +1,88 @@
+   * @param \Drupal\Core\PageCache\ResponsePolicy\KillSwitch $killSwitch
...
+  public function __construct(FlashBagInterface $flash_bag, KillSwitch $killSwitch) {
...
+    $this->killSwitch = $killSwitch;

Instead of injecting this we can have a private setter for this which can be removed once it is not needed anymore.

@xjm are you happy to forego the deprecation/trigger_error until we've removed all usages?

Instead of injecting this we can have a private setter for this which can be removed once it is not needed anymore.

No. This is a service that, currently, depends on another service. Please see #71 and #78 for more detailed explainations.

I'm posting a patch with out the Drupal::messenger() change in drupal_set_message() to see if its the cause of the fails.

Re-roll of #108

I'm posting a patch with out the Drupal::messenger() change in drupal_set_message() to see if its the cause of the fails.

I still think the 2 test failures in #96 are because those tests and trigger_error() don't mix.drupal_set_message() should definitely use \Drupal::messenger().

Could you remove the @deprecated verbiage and trigger_error() to see if that passes?

If it does, I think we should just add those as part of #2924538: [META] Remove all usages of drupal_set_message and drupal_get_messages.

@markcarver, the @trigger_error() is suppressed with an @, so it should never have any effect except for tests using the PHPUnit bridge. https://www.drupal.org/pift-ci-job/820652 shows two failures: one in InstallUninstallTest, and another in ExperimentalModuleTest. InstallUninstallTest is a simpletest web test, so I'm pretty sure the fails in it cannot be coming from the PHPUnit bridge. The message is also being added to the list of ignored warnings, so it should not raise a fail in PHPUnit tests using the bridge either.

It'd be simple to prove whether the @trigger_error() was causing fails: roll a version #96 without the @trigger_error() for testing, post it, and see if it passes. I suspect it still won't.

This should not be committed without the @trigger_error(). If somehow that was what was causing the fail, that'd indicate a deeper problem that we'd need to address.

InstallUninstallTest is unfortunately a bit of a nightmare to run locally, but ExperimentalModuleTest should be less painful. I'd recommend running the test with #96 locally and seeing what's actually in the verbose output on the steps right before the fails, and possibly stepping through it in a debugger if that doesn't work.

Oops, forgot to unassign. Answer to #106 is "No, keep the @trigger_error() and deprecation, and debug why the tests were actually failing." :)

Ok, I've been debugging this and this is the code that is causing the test failures:

+++ b/core/lib/Drupal/Core/Messenger/ChainedMessenger.php
@@ -0,0 +1,134 @@
+    // Shortcut if we have a messenger service.
+    if (isset($this->messengerService)) {
+      return $this->messengerService;
+    }

Basically, if the container gets rebuilt (thanks for pointing us in the right direction @dawehner!) then it doesn't merge in any messages from MemoryMessenger.

I think I've got a fix and will upload the patch shortly.

Thanks Mark!

+++ b/core/lib/Drupal/Core/Messenger/ChainedMessenger.php
@@ -99,19 +85,24 @@ public function deleteByType($type) {
+    $this->memoryMessenger = new MemoryMessenger();

This will create a new MemoryMessenger each time getMessenger() is called? Wouldn't messages be lost potentially?

This will create a new MemoryMessenger each time getMessenger() is called?

Heh, you're right! Needs to be wrapped in isset(). I'll upload another patch shortly with this change.

edit: after test results come back

Yay #116 passed!

Here's a change that fixes #117 and adds some better test coverage for determining which class is returned in ChainedMessenger.

Looks great! RTBC if green.

I suddenly realized that the MemoryMessenger is actually the wrong approach since it does not accurately provide 100% BC functionality.

If messages are added/retrieved between requests (prior to using the service), then they are lost. Despite current implementation, this is not the expected behavior and LegacyMessenger actually broke this aspect of the BC code.

Also, FlashBag uses $_SESSION['_sf2_flashes'] which is different from what core used to use: $_SESSION['messages'], so FlashBag can never be a 100% BC replacement.

3rd party code (like Drush) can and does included bootstrap.inc to utilize the drupal_set_message() and drupal_get_messages() procedural functions, prior to instantiating the container (e.g. creating a new site before a container exists).

If there is ever a new request, messages are lost because they're not using the previous stable method: $_SESSION['messages'] which can persist across requests.

I raised this in Slack and I have to steal a line from @kim.pepper and say that I discussed this with @larowlan :D

1. Rename MemoryMessenger to LegacyMessenger - edit: reuse existing name and clone the previous $_SESSION['messages'] implementation from proceedural functions
2. Rename all @internal tags on the BC messenger classes to @deprecated (they only exist to provide BC capabilities)
3. Add a @trigger_error whenever LegacyMessenger is used so we can start tracking who is actually using messages prior to a container being built (which may affect what we do in 9.x).

I meant to assign this to myself.

Actually, we may as well reuse LegacyMessenger instead of PreBootstrapMessenger since that is basically what this is and it reads better.

Here's a patch that reworks the existing LegacyMessenger class to mimic core's previous $_SESSION['messages'] code (#121). I also went ahead and removed a couple things:

• @internal from MessengerInterface and Messenger as was originally intended per: #2278383-230: Create an injectible service for drupal_set_message()
• BaseMessenger, this really didn't add much benefit given that it would have been deprecated (used for BC) and removed eventually anyway.

Yep

Not sure we can do this? From what I can tell, you're going to get a new ChainedMessenger each time you call \Drupal::messenger(), which will create a new LegacyMessenger each time, up until the container is available.

+++ b/core/lib/Drupal/Core/Messenger/LegacyMessenger.php
@@ -3,52 +3,81 @@
+    if (!isset($_SESSION['messages'])) {
+      $_SESSION['messages'] = [];
+    }
+    $this->messages = &$_SESSION['messages'];

Yes and that's fine because this will always reference $_SESSION['messages'] now which survives and is kind of like its own static.

1. +++ b/core/lib/Drupal/Core/Messenger/ChainedMessenger.php
   @@ -0,0 +1,134 @@
   +    foreach ($from->all() as $type => $messages) {
   ...
   +        $to->addMessage($message, $type);
   

   Isn't the order of messages important?

2. +++ b/core/lib/Drupal/Core/Messenger/LegacyMessenger.php
   @@ -3,52 +3,81 @@
   +  public function addMessage($message, $type = self::TYPE_STATUS, $repeat = FALSE) {
   

   Why don't we need a kill switch in this method?

RE: #127 We could make ChainedMessenger a single instance class just like \Drupal\Core\Site\Settings

Re #127 Ah ok. Makes sense then.

Re #130 We have \Drupal::messenger() as our single instance class. We really want people to inject MessengerInterface going forward.

#129.1: Yes and they're added in the order the were originally added.

#129.2: Because LegacyMessenger would only ever be used when there is no container available, which is required to actually use the kill switch. This class only exists to temporarily store messages until a container can be instantiated and then it switches over to it.

So by changing $messenger = new ChainedMessenger(); to $messenger = \Drupal::messenger(); in ChainedMessengerTest, the test fails. \Drupal::messenger() needs to be statically cached because otherwise the legacyMessenger property in ChainedMessenger would always be NULL and thus never transfer the service (#127 is correct).

Reverting that change and updating test to include testing \Drupal::messenger().

+++ b/core/lib/Drupal/Core/Messenger/LegacyMessenger.php
@@ -3,52 +3,81 @@
+    @trigger_error('\Drupal\Core\Messenger\LegacyMessenger is deprecated in Drupal 8.5.0 and will be removed before Drupal 9.0.0. Use \Drupal\Core\Messenger\Messenger instead. See https://www.drupal.org/node/2774931', E_USER_DEPRECATED);

Do we want to be a little more explicit with this error message? Maybe something along the lines of:

You have attempted to add or retrieve a message prior to the container being initialized. This functionality was deprecated in Drupal 8.5.0 and will be removed before Drupal 9.0.0. Please use the "messenger" service or report your legitimate use case at https://www.drupal.org/node/123456.

Obviously, we'd need to create a new issue to track these but it may be worthwhile considering that we really don't have any clue who is doing this aside from Drush. Then we can re-evaluate this functionality before actually removing it.

I still try to understand why we need to support pre container messages

Because the procedural functions drupal_set_message() and drupal_get_message() have existed for 14 years and have been heavily used by core, contrib and 3rd party alike.

These aren't some obscure functions that we can "fudge" with BC support. We know, for a fact, that Drush uses these functions prior to a container actually being available (i.e. creating a new site).

The reality is: we don't really know the extent of who or what uses these functions prior to the container being built/initialized because these functions never made the OO cut before 8.x was released. Thus, we will need 100% BC compliant code until we can answer this definitively and figure out a path moving forward in 9.x. This is why I raised #135.

but why can't we follow the strategy of HEAD aka. ignore messages?

Because HEAD is wrong. #2278383: Create an injectible service for drupal_set_message() introduced "ignoring messages" which breaks 100% BC. This issue needs to fix that.

---

+++ b/core/lib/Drupal.php
@@ -772,10 +765,7 @@ public static function time() {
+    return new ChainedMessenger();

+++ b/core/lib/Drupal/Core/Messenger/ChainedMessenger.php
@@ -99,10 +99,9 @@ public function getMessenger() {
+      $legacy_messanger = isset($this->legacyMessenger) ? $this->legacyMessenger : (new LegacyMessenger());

$this->legacyMessenger will always be NULL here because \Drupal::messenger() is creating a new ChainedMessenger instance each time. This is what @kim.pepper was saying in #127 and I misunderstood.

On a side note, this why I had originally intended the ChainedMessenger::getMessenger code to be in \Drupal::messenger() (#78). Keeping track of this static relationship between these two classes is confusing and requires that ChainedMessenger adds more unnecessary LOC to become a "proxy".

I really think that we should just remove ChainedMessenger altogether, which the exception of ChainedMessenger::getMessenger which should be moved into \Drupal::messenger. I don't see the benefit of having a dedicated class for this process.

I think removing ChainedMessenger entirely will help clear up some of this confusion regarding the static.

Hm, maybe if we just move the code in::getMessenger to LegacyMessenger, this would satisfy the "want for a separate class" and the static issue both.

Plus, this code will just disappear when LegacyMessenger is finally removed, which makes more sense.

I'm also adding the related issue for tracking usage of LegacyMessenger.

Ok, see if this makes more sense.

+++ b/core/lib/Drupal.php
@@ -757,4 +758,15 @@ public static function time() {
+    // @todo Replace with the Messenger service once LegacyMessenger has been removed in 9.0.0.

> 80

> 80

It's a @todo. It really shouldn't matter and IDEs like PHPStorm don't continue the @todo styling to newlines.

edit: sigh, it'll fail the coding standards automated tests though. this kind of stuff is ridiculous.