Create an injectible service for drupal_set_message()

1. +++ b/core/core.services.yml
   @@ -814,3 +814,5 @@ services:
   +  messenger:
   

   Can we call these flash messages, since that's what Symfony's session system calls them (and what we want to use internally eventually)?

2. +++ b/core/lib/Drupal/Core/Messenger/Messenger.php
   @@ -0,0 +1,57 @@
   +  public function setMessage($message, $type = self::STATUS, $repeat = FALSE) {
   

   This method isn't setting the message, it's adding another message to a list. Thus, addMessage().

3. +++ b/core/lib/Drupal/Core/Messenger/Messenger.php
   @@ -0,0 +1,57 @@
   +      drupal_page_is_cacheable(FALSE);
   

   This function needs to go away anyway; don't add another use of it.

4. +++ b/core/lib/Drupal/Core/Messenger/MessengerInterface.php
   @@ -0,0 +1,81 @@
   +   *   consistency with other messages, it should begin with a capital letter and
   

   Comma unnecessary.

Since it's using $_SESSION, I'm not sure if you can. :-( Not until we switch over to the Symfony session system.

Needed a re-roll, thought I'd poke at the unit tests.

Turns out Drush is calling drupal_get_messages() too early. I don't have time to follow up at this point.

Here's a POC.

This should fix the warnings.

+++ b/core/includes/bootstrap.inc
@@ -1221,12 +1215,24 @@ function drupal_set_message($message = NULL, $type = 'status', $repeat = FALSE)
+ *
+ * @deprecated Deprecated as of Drupal 8.0.
+ *   Use \Drupal::service('messenger')->getMessages() or
+ *   \Drupal::service('messenger')->getMessagesByType() instead.
  */
 function drupal_get_messages($type = NULL, $clear_queue = TRUE) {
-  if ($messages = drupal_set_message()) {
+  // Workaround for Drush.
+  if (!\Drupal::hasService('messenger')) {
+    return array();
+  }
+
+  /** @var \Drupal\Core\Messenger\MessengerInterface $messenger */
+  $messenger = \Drupal::service('messenger');
+
+  if ($messages = $messenger->getMessages()) {
     if ($type) {
       if ($clear_queue) {
-        unset($_SESSION['messages'][$type]);
+        $messenger->deleteMessagesByType($type);
       }
       if (isset($messages[$type])) {
         return array($type => $messages[$type]);
@@ -1234,7 +1240,7 @@ function drupal_get_messages($type = NULL, $clear_queue = TRUE) {

@@ -1234,7 +1240,7 @@ function drupal_get_messages($type = NULL, $clear_queue = TRUE) {
     }
     else {
       if ($clear_queue) {
-        unset($_SESSION['messages']);
+        $messenger->deleteMessages();
       }
       return $messages;
     }

Shouldn't all of drupal_get_messages() turn into a method call to the service? Otherwise this function is still necessary for useful behavior, which we do not want.

Can we do a sample conversion here while we're at it? For instance, this seems like a useful utility method for FormBase and ControllerBase; $this->addMessage() (or something). We don't need to convert all usages of dsm() here but having minimal integration sets the stage for the non-BC-breaking follow-up work.

There should be a trait too. In at least two contrib module in working on I have a wrapper method to dsm in a trait.

#16, The functionality of drupal_get_messages() has been broken up into distinct methods. The code there is just for compatibility. That logic isn't needed anymore once drupal_get_message() is dead.

Guess I'll bang on this some more.

Trait added and SpecialAttributesRouteSubscriber converted to use it.

+++ b/core/lib/Drupal/Core/Messenger/MessengerTrait.php
@@ -0,0 +1,63 @@
+/**
+ * Wrapper methods for \Drupal\Core\Messenger\Messenger.
+ */

The docblock should provide some direction for when to use it (and when not to). I am partial to the language introduced in #2282161: Split off link/url generation trait for obvious reasons. ;-) Of course, that makes listeners likely not a place that should use the trait; Controllers and Forms are the main users.

Aside from those points this looks good.

Re-roll of #19

I've added comments similar to what is in LinkGeneratorTrait as per @crell's comment in #20.

I also think this answers @Xano's issue in #22. Allowing the trait to work without DI is a huge advantage. We're using DI where ever possible in core, and advising contrib to use it with the comments.

RE: #21 Shouldn't we be injecting RequestStack and grabbing ->session() from that instead of using $_SESSION??

We're not yet converted over to the Symfony session handling AFAIK, so we can't use RequestStack / session(). :-(

I'm not sold on the trait, except insofar as it's something that could be argued belongs in ControllerBase and I would like to remove much of ControllerBase in favor of traits. :-) So, given that, should we add the trait to ControllerBase but NOT to a subscriber? (I agree that the subscriber shouldn't use the trait, as it's registered through the container at this point so it fails the test the trait says to use.)

1. +++ b/core/lib/Drupal/Core/Messenger/MessengerInterface.php
   @@ -0,0 +1,81 @@
   +
   +interface MessengerInterface {
   

   Let's not forgo some documentation here.

2. +++ b/core/tests/Drupal/Tests/Core/Messenger/MessengerTest.php
   @@ -0,0 +1,99 @@
   + * @coversDefaultClass \Drupal\Core\Messenger\Messenger
   + */
   +class MessengerTest extends UnitTestCase {
   

   Do we have an idea about an @group?

3. +++ b/core/tests/Drupal/Tests/Core/Messenger/MessengerTest.php
   @@ -0,0 +1,99 @@
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public static function getInfo() {
   +    return array(
   +      'description' => '',
   +      'name' => '\Drupal\Core\Messenger\Messenger unit test',
   +      'group' => 'Messenger',
   +    );
   +  }
   

   Yeah, we can drop that shit now!

4. +++ b/core/tests/Drupal/Tests/Core/Messenger/MessengerTest.php
   @@ -0,0 +1,99 @@
   +  /**
   +   * Tests the messenger.
   +   */
   

   We have to specify @covers in order to let code coverage see that.

5. +++ b/core/tests/Drupal/Tests/Core/Messenger/MessengerTraitTest.php
   @@ -0,0 +1,85 @@
   +/**
   + * @coversDefaultClass \Drupal\Core\Messenger\MessengerTrait
   + */
   +class MessengerTraitTest extends UnitTestCase {
   ...
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public static function getInfo() {
   +    return array(
   +      'name' => 'Messenger trait',
   +      'description' => 'Tests the messenger trait.',
   +      'group' => 'Messenger',
   +    );
   +  }
   

   See above

6. +++ b/core/tests/Drupal/Tests/Core/Messenger/MessengerTraitTest.php
   @@ -0,0 +1,85 @@
   +    $this->messengerTrait->setMessenger($stub);
   +
   +    $method = $this->reflection->getMethod('getMessenger');
   +    $method->setAccessible(TRUE);
   

   I totally think this is okay to do here!

Re-rolled for #1825952: Turn on twig autoescape by default and fixes for #25

Hmmm. Patch doesn't look right.

This is a straight re-roll of #23 without fixes in in #25.

Can we drop the trait for now and punt that to a spinoff issue to debate separately? The rest here we all seem to be OK with and we should get it in ASAP.

(FTR, I am not a big fan of the trait either as it should, really, only ever be used in controllers and forms. Other code shouldn't be using it, period. But I'm not going to die on that hill in this case. Other trait proposals I might...)

Trait be gone! (whoosh)

I also fixed an issue merging in the autoescape patch.

Fixes test failures.

I'm not 100% sure of how the SafeMarkup::isSafe($message) part works, so need someone to review that.

+++ b/core/includes/bootstrap.inc
@@ -962,9 +952,22 @@ function drupal_set_message($message = NULL, $type = 'status', $repeat = FALSE)
+ * @deprecated Deprecated as of Drupal 8.0.
+ *   Use \Drupal::service('messenger')->getMessages() or
+ *   \Drupal::service('messenger')->getMessagesByType() instead.

In that case the marking safe and all that is happening below here in drupal_get_messages needs to be ported over to that.

The isSafe call is okay though.

Sorry Xano :-( All yours.

+++ b/core/lib/Drupal/Core/Messenger/Messenger.php
@@ -0,0 +1,68 @@
+    return isset($_SESSION['messages']) ? $_SESSION['messages'] : array();

This should do the same as drupal_get_messages() and process the 'safe' flag, then return a list of strings.

Explanation:

> It also seems silly we have to fix a Twig issue here, but I have no practical experience with auto-escape, so I may be wrong.

You need to ensure safe strings are persisted across page requests. This is Drupals new autoescape security model.

How you do that is up to your implementation, the current implementation uses arrays(), which is the reason why $_SESSION must not be returned directly from getMessages or getMessagesByType(), but the processing from drupal_get_messages() needs to be ported over.

That means:

Before storing in $_SESSION -> Detect which strings are safe and store somewhere, use SafeMarkup::isSafe()
After retrieving from $_SESSION -> Detect which strings have been safe in the previous request and mark them again as safe using SafeMarkup::set().

Does that make things clearer?

@Xano: no, I'm not sure either. They were introduced with the auto-escape patch.

This looks great now! Thanks so much!

+++ b/core/lib/Drupal/Core/Messenger/Messenger.php
@@ -0,0 +1,110 @@
+    $this->pageCacheKillSwitch = $page_cache_kill_switch;

No earthly idea what this is. It is never used...

Manual install is working for me, however, drush install fails when calling drupal_get_message().

I've created a separate issue #2347287: Provide a trait for setting messages for creating a trait that wraps drupal_set_message. We can swap it with this service once it is in.

Awesome work getting this green, Xano!

I added a second messenger class that stores messages in one of its properties, so it can only be used for setting messages that will be retrieved within the same request.

Is this used anywhere? Or is it meant to be a swap in replacement for the sessionmessenger?

+++ b/core/includes/bootstrap.inc
@@ -938,20 +927,25 @@ function drupal_set_message($message = NULL, $type = 'status', $repeat = FALSE)
+ *
+ *

Nitpick: Double space

+++ b/core/lib/Drupal/Core/Messenger/StaticMessenger.php
@@ -0,0 +1,120 @@
+  /**
+   * The page caching kill switch.
+   *
...
+  protected $messages = array();

This comment does not look right in this context.

The static manager is nice, but it would be even nicer if the normal messenger would extend the static messenger class as commonly done elsewhere.

Currently too much code is duplicated.

I also verified the autoescape implementation and its perfectly implemented (as I was originally pinged on that).

Thanks so much!

I don't know if it's not usable yet with the ongoing session work, but as a reminder Symfony has a session-based message system called flash messages that was enhanced specifically for Drupal, 2 years ago. We still haven't gotten around to leveraging it. :-) Can we yet?

@Crell: #2229145: Register symfony session components in the DIC and inject the session service into the request object introduces the necessary infrastructure but that is blocked by #2230121: Remove exit() from FormBuilder.

Flash messages are part of HttpFoundation. We already have the code. We're just not using it. Repeat prevention sounds like something to add upstream if possible.

Lets fix the nitpicks and this should be RTBC in my book. IMHO we can do the conversion to swap out the implementation with Symfony flash messages later - the service is simple enough and this interface nice.

The only two nitpicks left are:

* The page caching kill switch.

is describing a method having nothing to do with that - might not be explicit from the context.

Double Space from #66.

I meant to say that the code duplication was not worth the effort for the base class (that was not a nitpick, so I thought I was clear - why I was not ...)

So only two typos, and should be good to go.

Sorry for the confusion.

+++ b/core/lib/Drupal/Core/Messenger/StaticMessenger.php
@@ -0,0 +1,120 @@
+class StaticMessenger implements MessengerInterface {
+
+  /**
+   * The page caching kill switch.
+   *
+   * @var array[]
+   *   Keys are either self::STATUS, self::WARNING, or self::ERROR. Values are
+   *   arrays of arrays with the following keys:
+   *   - message (string): the message.
+   *   - safe (bool): whether the message is marked as safe markup.
+   */
+  protected $messages = array();
+

Here it says

'The page caching kill switch.'

which is wrong and what I meant.

Back to RTBC then, thanks so much

We should deprecate till Drupal 9.0 now and not 8.0.

+++ b/core/includes/bootstrap.inc
@@ -892,20 +881,25 @@ function drupal_set_message($message = NULL, $type = 'status', $repeat = FALSE)
+  // Workaround for code that can not check if the service exists.
+  if (!\Drupal::hasService('messenger')) {
+    return array();
+  }

I don't think we should commit this with the workaround. This is basically hiding dependency problems. We should convert all the code this fails because of this problem in the this patch.

diff --git a/core/includes/bootstrap.inc b/core/includes/bootstrap.inc
index 74468d1..34054a2 100644
--- a/core/includes/bootstrap.inc
+++ b/core/includes/bootstrap.inc
@@ -7,7 +7,6 @@
 use Drupal\Component\Datetime\DateTimePlus;
 use Drupal\Component\Utility\Crypt;
 use Drupal\Component\Utility\Environment;
-use Drupal\Component\Utility\SafeMarkup;
 use Drupal\Component\Utility\String;
 use Drupal\Component\Utility\Unicode;
 use Drupal\Core\DrupalKernel;

We can remove SafeMarkup from bootstrap.inc now.

diff --git a/core/lib/Drupal/Core/Messenger/SessionMessenger.php b/core/lib/Drupal/Core/Messenger/SessionMessenger.php
index cbe4c33..1e4710f 100644
--- a/core/lib/Drupal/Core/Messenger/SessionMessenger.php
+++ b/core/lib/Drupal/Core/Messenger/SessionMessenger.php
@@ -102,7 +102,7 @@ public function deleteMessagesByType($type) {
   }
 
   /**
-   * Proccesses safe markup.
+   * Processes safe markup.
    *
    * @param array[]
    *   An array of arrays with the following keys:

Spelling mistake.

Here is a review ...

1. +++ b/core/includes/bootstrap.inc
   @@ -892,20 +881,25 @@ function drupal_set_message($message = NULL, $type = 'status', $repeat = FALSE)
   +  // Workaround for code that can not check if the service exists.
   +  if (!\Drupal::hasService('messenger')) {
   +    return array();
   +  }
   

   If we need this, there might be good reasons, so can we document when we need this? Maybe early installer?

2. +++ b/core/includes/bootstrap.inc
   @@ -892,20 +881,25 @@ function drupal_set_message($message = NULL, $type = 'status', $repeat = FALSE)
   +
   +  /** @var \Drupal\Core\Messenger\MessengerInterface $messenger */
   +  $messenger = \Drupal::service('messenger');
   +
   

   In general I think drupal_set_message() is something you tend to use REALLY often, right? Most form submit methods have some for example. Did you considered to introduce \Drupal::messenger() or \Drupal::message()?

3. +++ b/core/lib/Drupal/Core/Messenger/MessengerInterface.php
   @@ -0,0 +1,81 @@
   +
   +interface MessengerInterface {
   

   Can we have some general description like: Allows to add messenges visible by the user to the site?

4. +++ b/core/lib/Drupal/Core/Messenger/MessengerInterface.php
   @@ -0,0 +1,81 @@
   +   * @param string $message
   +   *   (optional) The translated message to be displayed to the user. For
   +   *   consistency with other messages, it should begin with a capital letter
   +   *   and end with a period.
   ...
   +  public function addMessage($message, $type = self::STATUS, $repeat = FALSE);
   

   Given that SafeMarkup problem, did we thought about also providing support for render arrays and so directly use '#type' => 'inline_template'?

5. +++ b/core/lib/Drupal/Core/Messenger/StaticMessenger.php
   @@ -0,0 +1,120 @@
   +      $this->pageCacheKillSwitch->trigger();
   

   Its kind of odd from an architecture point of view that each implementation has to take care of that. Maybe you could at least have a common base class to simplify stuff like processMessages etc?

6. +++ b/core/tests/Drupal/Tests/Core/Messenger/SessionMessengerTest.php
   @@ -0,0 +1,131 @@
   +class SessionMessengerTest extends UnitTestCase {
   +
   

   One thing I don't see yet, but is certainly REALLY important is a test for checking that SafeMarkup/escaping is done properly

7. +++ b/core/tests/Drupal/Tests/Core/Messenger/SessionMessengerTest.php
   @@ -0,0 +1,131 @@
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public static function getInfo() {
   +    return array(
   +      'description' => '',
   +      'name' => '\Drupal\Core\Messenger\Messenger unit test',
   +      'group' => 'Messenger',
   +    );
   +  }
   
   +++ b/core/tests/Drupal/Tests/Core/Messenger/StaticMessengerTest.php
   @@ -0,0 +1,97 @@
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public static function getInfo() {
   +    return array(
   +      'description' => '',
   +      'name' => '\Drupal\Core\Messenger\Messenger unit test',
   +      'group' => 'Messenger',
   +    );
   +  }
   +
   

   We can drop those

8. +++ b/core/tests/Drupal/Tests/Core/Messenger/SessionMessengerTest.php
   @@ -0,0 +1,131 @@
   +
   +    $this->existingSession = isset($_SESSION) ? $_SESSION : NULL;
   +    $_SESSION = array();
   +  }
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function tearDown() {
   +    if ($this->existingSession !== NULL) {
   +      $_SESSION = $this->existingSession;
   +    }
   +    else {
   +      unset($_SESSION);
   +    }
   

   Its kinda odd, that you have to do that on your own. If you read about https://phpunit.de/manual/current/en/appendixes.annotations.html#appendi... and https://phpunit.de/manual/current/en/fixtures.html#fixtures.global-state it is a pitty that $_SESSION is explicit not listed here.

9. +++ b/core/tests/Drupal/Tests/Core/Messenger/SessionMessengerTest.php
   @@ -0,0 +1,131 @@
   +
   +    // Test deleting messages of a certain type.
   +    $this->assertEquals($this->messenger->deleteMessagesByType($type_a), $this->messenger);
   +    $this->assertEquals($this->messenger->getMessages(), array(
   +      $type_b => array($message_b),
   +    ));
   ...
   +    // Test deleting all messages.
   +    $this->assertEquals($this->messenger->deleteMessages(), $this->messenger);
   +    $this->assertEquals($this->messenger->getMessages(), array());
   +  }
   +
   

   You could split this test up in multiple test methods using https://phpunit.de/manual/4.3/en/appendixes.annotations.html#appendixes....

See #2073817-9: [META] Remove drupal_set_message()

#1251960: harmonize severity/type in watchdog and drupal_set_message has been around for some time and suggested changes to the '$type' parameter that might be worth considering. If we're going to deprecate drupal_set_message() altogether in due course, it is probably worth considering whether any other API changes are worthwhile to do at the same time.

I think its wrong to not do it now, if folks are interested to work on it.

a) We want to make the life easy for people, if this means providing a service, then we should do that. It has obvious advantages like unit-testabilty,
b) Why should drupal_set_message() be excluded but #2426805: Modernize drupal_get_destination() got in?
c) The session work moved forward in the meantime, so we could solve things maybe better now.

I agree with dawehner. Improved testability via decoupling is a benefit for everyone working on core, because it means more and more code becomes easy and fast to test. It only takes a single drupal_set_message() call to make a proper unit test with PHPUnit impossible/prohibitively hard.

We obviously can't remove the existing function at this point in the cycle, but no one is proposing that. Just that it, like so many other functions, front for a sustainable version that can carry us through the full 8.x lifespan.

While we could do much of this work in 8.1, 8.0 is what most books and documentation will get written for. Getting certain things *right* before they're captured for all time in tree carcasses is important to ensure the community learns good practices from the beginning rather than needing to relearn them again 6 months later, because most won't. It's also another point for non-Drupal PHP devs to look at Drupal and go "oh, it's still lame, never mind"; we want to avoid that.

You had me at 're-open'.

Updated issue summary, and added beta stage evaluation (very similar to the one in #2426805: Modernize drupal_get_destination() !)

Updated issue title, as we are no longer proposing to replace drupal_set_message().

Would like to get some feedback from committers as to why #2426805: Modernize drupal_get_destination() passed the beta stage evaluation and how that relates to this issue.

I think it's a good idea and we can do it without BC break as well by keeping wrapper function.

Is anyone brave enough to change this back to 'active'?

I don't have any problem with doing that.

So for me the reason #2426805: Modernize drupal_get_destination() was permitted is that it directly relates to the routing system that we've been modernizing through the release and its testability. The message system has not been modernized and is a bigger problem space on its own than drupal_get_destination(). Currently there are 48 calls to drupal_get_destination - there are 417 to d_s_m(). The call I made on drupal_get_destination() was that the disruption (because there is disruption and risk even with a bc layer) was worth it. I'm not sure I could or would make the same argument here.

Thanks @alexpott and @dawehner.

Following the beta evaluation linked in the summary:

1. This issue is not a feature.
2. This issue is not unfrozen (does not only affect strings, CSS, tests, etc.).
3. This issue is not critical.
4. This issue does not include any prioritized changes (usability, accessibility, security, performance, follow-up to recent critical or major work, tagged for backport).
5. This issue is not major.
6. This issue does not have a maintainer signoff for "reducing fragility" (code not being easily unit testable is not a case for that).

It does not matter whether or not it is disruptive -- following the flowchart, a normal task with no prioritized changes is postponed. Therefore, this issue should remain postponed to 8.1.x. If anyone can make a case for why any of the points in the list above are not true, please do so in the summary and we can discuss it further, but I think it'd be better to focus resources elsewhere. Thanks!

The only test that has a fake implementation of drupal_set_message() is AggregatorPluginSettingsBaseTest. I think we should just convert that to a KernelTestBase test and move on.

I'll try to make another point, DX consistency.

I had a look through common.inc and bootstrap.inc and had a look which don't have yet a more modern variant yet.

drupal_set_message() is from my point of view one of the view remaining methods which are really often used (proven by alex in #2278383-97: Create an injectible service for drupal_set_message().
There is drupal_get_path() / drupal_get_filename() but they are really not often needed, now that many things are classes.

Hello all,

FYI, I took this patch and added it to the 8.x-1.x branch of Service Container.

Link of the pull request: https://github.com/LionsAd/service_container/pull/58

Re - #101:

Due to the not-sure-this-will-the-final-implementation-in-Drupal-8.1 we instead used our LegacyMessenger from Drupal 7 and just used the MessengerInterface.

Agreed we should make dsm into a service, but the test problems are somewhat fixed in this WIP: #2301873: Use ExtensionDiscovery to register test namespaces for running PHPUnit Patch in #8.

Let's reroll it, fix a couple of things and try to get the call rolling again.

Let's see whether this helps

1137, 1138, let's see, maybe the next patch will be 1139.

Less failures

+++ b/core/lib/Drupal/Core/Messenger/SessionMessenger.php
@@ -0,0 +1,94 @@
+    if (!isset($_SESSION['messages'][$type])) {
+      $_SESSION['messages'][$type] = [];
...
+      $_SESSION['messages'][$type][] = $message;
...
+    $messages = isset($_SESSION['messages']) ? $_SESSION['messages'] : [];
...
+    $messages = isset($_SESSION['messages']) && isset($_SESSION['messages'][$type]) ? $_SESSION['messages'][$type] : [];
...
+    unset($_SESSION['messages']);
...
+    unset($_SESSION['messages'][$type]);

Can we use session parameter bag here?

Thank you for your review @jibran
Well I considered this issue as not changing the internal behaviour at all, but rather providing just the service. We can afterwards try to change the internals.

@jibran
To clarify my answer a bit more ... this patch is a patch that optimizes for reviewability. It changes one thing, rather than two at the same time. There is no reason we cannot do the other step, but we already gain a lot by just going the first step.

Thanks for the explanation @dawehner.

1. +++ b/core/lib/Drupal/Core/Messenger/SessionMessenger.php
   @@ -0,0 +1,94 @@
   + * Provides a session-based messenger.
   
   +++ b/core/lib/Drupal/Core/Messenger/StaticMessenger.php
   @@ -0,0 +1,104 @@
   + * Provides a messenger that stores messages for this request only.
   

   Let's add some explanation here for the documentation

2. +++ b/core/lib/Drupal/Core/Messenger/StaticMessenger.php
   @@ -0,0 +1,104 @@
   +      $this->pageCacheKillSwitch->trigger();
   

   Just wondering, do we really need kill switch for static messenger?

+1 to #117 and first providing the interface, then changing the implementation!

Thank you @jibran, let's document a bit more.

Just wondering, do we really need kill switch for static messenger?

Well, the page output still varies so we should not cache.

Thank you for improving the docs.

1. +++ b/core/lib/Drupal/Core/Messenger/MessengerInterface.php
   @@ -4,6 +4,9 @@
   + * Examples for these messages are for example that a specific content got
   + * saved.
   

   This doesn't seem right. What are we trying to say here?

2. +++ b/core/lib/Drupal/Core/Messenger/SessionMessenger.php
   @@ -8,6 +8,9 @@
   + * This is the default implementation used by Drupal to store message to the
   + * user.
   

   s/message to/messages for

Well, the page output still varies so we should not cache.

Right! but isn't the page uncached already?

I found it strange that $_SESSION is still in the latest patch.

RE:#123.6 I think it is more of a personal preference but returning $this has its own benefit and I kind a like it so let's not bikeshed on it here in this issue. I think we are already discussing it in #2134513: [policy, no patch] Trait strategy.

#124: We are doing baby steps here. Once the interface is stable the implementation can be enhanced / changed / debated and bike shedded internally.

#126, #124
I hope we can agree on this. Here is a follow up: #2760167: Add \Drupal\Core\Messenger\Messenger

This doesn't seem right. What are we trying to say here?

Improved this.

Right! but isn't the page uncached already?

Its just for the installer so it really doesn't matter, but IMHO its more "correct" to just do it. It also gives a good examples in case someone comes up with an alternative implementation.

@jibran already pointed out this does not make sense in English (the sentence was likely edited a couple of times, it happens), but I also think we should describe the type of messages in more detail. This interface is responsible for handling runtime messages to individual users, as opposed to log messages or private messages, for instance.

I changed the text again to include a concrete example, which IMHO, should make it clearer.

I propose prefixing the constants with TYPE_ or something similar, for readability, so we can easily reference them in documentation, and they will not collide with possible future constants.

Surem why not!

$message is string|\Drupal\Core\StringTranslation\TranslatableMarkup

Do we do that anywhere else in core? MarkupInterface is designed to be hidden from the user.

If we prefix the constants like suggested earlier, we can simplify this comment by replacing individual constant names with self::TYPE_*

IMHO as we have just 3 examples we should list them.

I have come to dislike fluent interfaces since I first worked on this issue, since it's hard to mock and decorate, and does not provide too many DX improvements for production code. Can we change these methods to not return anything at all?

I see your point, especiall on interfaces which are not meant to be called more than once. On the other hand this would break quite the consistency we have now in many places all over core. What about opening up a dedicated discussion for future API designs?

Do we support MarkupInterface or TranslatableMarkup?

If you read the code above its actually a Markup object at this point in time.

Can we somehow replace the kill switch by exposing cacheability data?

No we cannot. Page cache is designed to not use cacheability metadata, as this would be something outside of the HTTP layer.

Here is the patch as well.

Can we somehow replace the kill switch by exposing cacheability data?

No we cannot. Page cache is designed to not use cacheability metadata, as this would be something outside of the HTTP layer.

That's a little bit over-simplifying. In this case, the cacheability metadata we need to bubble is max-age=0. Not cache tags/contexts. So in this case, we could actually make that work. But until we have #2352009: [pp-3] Bubbling of elements' max-age to the page's headers and the page cache, we can't. That was deemed to big a change before D8.0.0, so we didn't get that.

The real problem here is that the thing calling drupal_set_message() is not guaranteed to also be directly rendering something into the response that can set max-age=0.

Ideally, you wouldn't use a kill switch, but a \Drupal\Core\PageCache\ResponsePolicyInterface. And since the message is associated with a response, ideally you'd have #attached['messages'], just like you have #attached['library'], #attached['http_header'], et cetera. Then the response policy could do something like:

  public function check(Response $response, Request $request) {
    if ($response instanceof HtmlResponse && !empty($response->getAttachments()['messages'])) {
      return static::DENY;
    }
  }

However, since this issue continues to use $_SESSION, perhaps that's something for #2760167: Add \Drupal\Core\Messenger\Messenger.

#130: However we both know that the kill-switch in drupal_set_message() can be pretty useless - if the request is forwarded to another URL e.g..

However given the complexity of the space, this should definitely be a separate follow-up issue.

Right this discussion is quite out of scope of this issue. :)

I'm not sure if we do, but since we don't technically just work with strings, we should document it. Let's put it another way: if we'd use a string type hint, the type check would fail on translatable markup.

Well, we use string in let's say 1mio other places. I don't really mind, except that this is hard to update in the future. Nowhere near a blocker, though. I hope we don't add another type rather soon than later :)

Back to needs review ... I hope that's fair, as I have given a point.

Now that I commented I realized, that drupal_set_message() did the same, so let's do the same here as well :P

Is [][] a thing? I can't find it in our documentation standards, but I'd be happy to use it!

I've seen it in other places, and well it kinda just makes sense. type[] is an array of type. So when type=type2[], then type[] is an array of array of type2.

It's exciting to see this issue RTBC!

Tagging for a framework manager's review given the architectural importance of this.

Edit: @Xano, as of last I checked, only committers could actually save issue credits. A new feature could have been deployed, but in case it didn't seem to work I wanted to explain why.

+++ b/core/includes/bootstrap.inc
@@ -487,12 +470,24 @@ function drupal_set_message($message = NULL, $type = 'status', $repeat = FALSE)
+  // Workaround for code that can not check if the service exists.
+  if (!\Drupal::hasService('messenger')) {
+    return [];
+  }

This just eats the message, whereas the previous code would put it into $_SESSION always. Are we actually hitting this condition and if so what's getting lost?

That workaround was brought up in #40, #80 and #81 as well - let's remove it from the patch.

Removed from the patch re #148 Let's see what happens

Green! Looks that workaround for some contrib that trying access message in early bootstrap with incomlete container

Discussed with @catch. Neither of us have any framework concerns with this patch. Using $_SESSION is mandatory for BC unfortunately but it's a simple refactor for Drupal 9.

Committed ee22272 and pushed to 8.2.x. Thanks!

After I saw this fixed I jumped to read the CR :) Or is not needed because we are only deprecating drupal_set_message()?

#153 A change record should be good indeed.

I'' write one

Can someone have a look at it https://www.drupal.org/node/2774931 and publish it if it looks okayish?

+++ b/core/includes/bootstrap.inc
@@ -487,12 +470,24 @@ function drupal_set_message($message = NULL, $type = 'status', $repeat = FALSE)
+  // Workaround for code that can not check if the service exists.
+  if (!\Drupal::hasService('messenger')) {
+    return [];
+  }

So without this code drush can not install Drupal 8. It would have been better if the comment had given the example because this is probably why this was here.

If we do have this workaround in core then we should try to set up a test for it.

I think I implemented that hack, and if I remember, it was for Drush.

Can't we just fix Drush?

@twistor well yes but then that needs deploying etc... and this won't make it into 8.2.x. I think the workaround is okay - it just needs better docs and maybe a test. You could use an isolated PHPUnit test to do this for example.

Or a kernel test where you remove the container from \Drupal

Sure, let's open a follow up #2775211: [PP-1] Test and document why drupal_set_message() has a workaround for a missing container

drupal_get_message() is also used to clear messages in $_SESSION, so if we have the workaround at all, it should retain bc by handling that case, not just return [];

Can someone actually explain why this was rolled back?

#2278383-158: Create an injectible service for drupal_set_message() seems not like a real explanation for that.