Problem/Motivation

Currently drupal_set_message() is called inside OO code, which makes it hard to unit test. There are examples in core where this method is being stubbed out, but it is complicated. This dissuades module developers from unit testing their code.

Proposed resolution

Provide a new service to inject into OO code. Keep drupal_set_message() for backwards compatibility, but deprecate it.

Remaining tasks

Do it.

User interface changes

None.

API changes

No changes. New API.

Original Description

Convert drupal_set_message() and drupal_get_message() to a service, so it can be injected and unit tested, and the implementation can be swapped.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task, because we want to able to allow proper testability of our code.
Issue priority Normal, because the impact is not that high
Disruption No disruption, because the BC layer is kept
CommentFileSizeAuthor
#232 2278383-232-interdiff.txt2.11 KBkim.pepper
#232 2278383-232.patch17.59 KBkim.pepper
#224 2278383-224-interdiff.txt2.99 KBkim.pepper
#224 2278383-224.patch18.06 KBkim.pepper
#223 2278383-223-interdiff.txt1.71 KBkim.pepper
#223 2278383-223.patch18.01 KBkim.pepper
#220 2278383-220.patch17.78 KBdawehner
#217 interdiff-2278383.txt2.73 KBdawehner
#217 2278383-217.patch17.78 KBdawehner
#214 interdiff-212-214.txt1.11 KBJo Fitzgerald
#214 2278383-214.patch16.07 KBJo Fitzgerald
#212 interdiff-2278383.txt7.36 KBdawehner
#212 2278383-212.patch16.1 KBdawehner
#206 2278383-206-interdiff.txt1 KBkim.pepper
#206 2278383-206.patch15.96 KBkim.pepper
#201 interdiff.txt1.1 KBpritish.kumar
#201 2278383-201.patch15.91 KBpritish.kumar
#199 interdiff-2278383.txt488 bytesdawehner
#199 2278383-199.patch15.92 KBdawehner
#197 interdiff-2278383.txt4.78 KBdawehner
#197 2278383-197.patch15.92 KBdawehner
#195 interdiff-2278383.txt0 bytesdawehner
#195 2278383-195.patch15.93 KBdawehner
#193 interdiff-2278383.txt1.4 KBdawehner
#193 2278383-193.patch19.03 KBdawehner
#191 2278383-191.patch18.99 KBdawehner
#191 interdiff-2278383.txt18.7 KBdawehner
#190 create_an_injectible-2278383-190.patch22.78 KBjibran
#190 interdiff-77fa01.txt1.33 KBjibran
#188 create_an_injectible-2278383-188.patch22.83 KBjibran
#188 interdiff-fcd2a4.txt579 bytesjibran
#186 create_an_injectible-2278383-186.patch22.89 KBjibran
#186 interdiff-88e757.txt728 bytesjibran
#185 create_an_injectible-2278383-185.patch22.71 KBJo Fitzgerald
#149 create_an_injectible-2278383-149.patch22.1 KBjoelpittet
#149 interdiff.txt660 bytesjoelpittet
#137 interdiff.txt1.3 KBdawehner
#137 2278383-137.patch22.24 KBdawehner
#129 interdiff.txt5.11 KBdawehner
#129 2278383-129.patch22.11 KBdawehner
#121 interdiff.txt963 bytesdawehner
#121 2278383-121.patch22.06 KBdawehner
#115 interdiff.txt3.21 KBdawehner
#115 2278383-115.patch21.88 KBdawehner
#113 interdiff.txt1.72 KBdawehner
#113 2278383-113.patch22.55 KBdawehner
#110 2278383-110.patch22.28 KBdawehner
#108 2278383-108.patch22.46 KBdawehner
#78 drupal_2278383_78.patch26.36 KBXano
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,035 pass(es). View
#78 interdiff.txt1.13 KBXano
#75 drupal_2278383_75.patch26.36 KBXano
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,027 pass(es). View
#65 drupal_2278383_65.patch26.35 KBXano
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,794 pass(es). View
#65 interdiff.txt822 bytesXano
#63 drupal_2278383_63.patch26.34 KBXano
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View
#63 interdiff.txt808 bytesXano
#61 drupal_2278383_61.patch26.34 KBXano
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh --clean. View
#61 interdiff.txt7.09 KBXano
#59 drupal_2278383_59.patch19.24 KBXano
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,340 pass(es), 385 fail(s), and 25 exception(s). View
#59 interdiff.txt3.44 KBXano
#57 drupal_2278383_57.patch18.77 KBXano
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,335 pass(es), 386 fail(s), and 25 exception(s). View
#57 interdiff.txt3.29 KBXano
#55 drupal_2278383_55.patch18.35 KBXano
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,734 pass(es), 0 fail(s), and 16 exception(s). View
#55 interdiff.txt665 bytesXano
#50 drupal_2278383_49.patch18.22 KBXano
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View
#49 interdiff.txt2.44 KBXano
#45 drupal_2278383_45.patch18.45 KBXano
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View
#40 drupal_2278383_40.patch13.91 KBXano
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,231 pass(es). View
#40 interdiff.txt614 bytesXano
#36 interdiff.txt2.28 KBkim.pepper
#36 2278383-drupal-set-message-36.patch13.79 KBkim.pepper
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,192 pass(es). View
#34 interdiff.txt8.11 KBkim.pepper
#34 2278383-drupal-set-message-34.patch13.3 KBkim.pepper
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,216 pass(es), 14 fail(s), and 0 exception(s). View
#28 2278383-drupal-set-message-28.patch16.63 KBkim.pepper
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,326 pass(es), 1,385 fail(s), and 13,510 exception(s). View
#26 2278383-drupal-set-message-26.patch16.7 KBkim.pepper
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,157 pass(es), 1 fail(s), and 0 exception(s). View
#23 interdiff.txt649 byteskim.pepper
#23 2278383-drupal-set-message-23.patch16.28 KBkim.pepper
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,392 pass(es). View
#19 drupal_2278383_19.patch16.02 KBtwistor
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,296 pass(es). View
#19 interdiff.txt6.45 KBtwistor
#15 drupal_2278383_15.patch9.64 KBtwistor
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,295 pass(es). View
#15 interdiff.txt701 bytestwistor
#13 drupal_2278383_13.patch9.57 KBtwistor
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,235 pass(es), 5 fail(s), and 1,916 exception(s). View
#13 interdiff.txt1.47 KBtwistor
#11 drupal_2278383_11.patch9.43 KBtwistor
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View
#7 drupal_2278383_7.patch9.01 KBXano
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View
#7 interdiff.txt6.82 KBXano
#2 drupal_2278383_1.patch7.6 KBXano
#1 drupal_2114823_27.patch15.97 MBXano
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

Xano’s picture

Status: Active » Needs work
FileSize
15.97 MB

The tests will need to be finished. I am having some trouble with that, as PHPUnit prints output before the session can be started in the test itself.

Xano’s picture

FileSize
7.6 KB

#1 accidentally contained the wrong patch.

Crell’s picture

  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.

Xano’s picture

Great, thanks for the review! Do you have any suggestions on how to unit test this?

Crell’s picture

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

Xano’s picture

Flash messages are part of Symfony's session component. However, drupal_set_message() uses the session, but is not part of the session system. Renaming this code may therefore send the wrong message.

Xano’s picture

Status: Needs work » Needs review
FileSize
6.82 KB
9.01 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View

Status: Needs review » Needs work

The last submitted patch, 7: drupal_2278383_7.patch, failed testing.

Xano’s picture

Status: Needs work » Needs review

7: drupal_2278383_7.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 7: drupal_2278383_7.patch, failed testing.

twistor’s picture

Status: Needs work » Needs review
FileSize
9.43 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View

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

Status: Needs review » Needs work

The last submitted patch, 11: drupal_2278383_11.patch, failed testing.

twistor’s picture

Status: Needs work » Needs review
FileSize
1.47 KB
9.57 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,235 pass(es), 5 fail(s), and 1,916 exception(s). View

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.

Status: Needs review » Needs work

The last submitted patch, 13: drupal_2278383_13.patch, failed testing.

twistor’s picture

Status: Needs work » Needs review
FileSize
701 bytes
9.64 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,295 pass(es). View

This should fix the warnings.

Crell’s picture

+++ 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.

larowlan’s picture

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.

twistor’s picture

Assigned: Xano » twistor

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

twistor’s picture

FileSize
6.45 KB
16.02 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,296 pass(es). View

Trait added and SpecialAttributesRouteSubscriber converted to use it.

Crell’s picture

+++ 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.

Xano’s picture

+++ b/core/lib/Drupal/Core/Messenger/Messenger.php
@@ -19,6 +19,9 @@ class Messenger implements MessengerInterface {
+    if (!isset($_SESSION)) {
+      $_SESSION = array();
+    }

I am not a session guru, but should this superglobal not be set automatically once the session has been started?

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.

drupal_set_message() used to do too many things at once, partly because there was no central location to store the messages. This issue does not only convert messages to a service, but also improves the code by using classes' ability to store state in themselves and by splitting functionality up into different classes for better DX.

Xano’s picture

@twistor: I am generally not in favor of adding traits that aid in setting dependencies on classes, as it does not motivate people to use dependency injection at all. Why did you add one, and why did you use it for SpecialAttributesRouteSubscriber? There is absolutely no reason that class shouldn't use constructor injection.

kim.pepper’s picture

FileSize
16.28 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,392 pass(es). View
649 bytes

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

Crell’s picture

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.)

dawehner’s picture

  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!

kim.pepper’s picture

FileSize
16.7 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,157 pass(es), 1 fail(s), and 0 exception(s). View

Status: Needs review » Needs work

The last submitted patch, 26: 2278383-drupal-set-message-26.patch, failed testing.

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
16.63 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,326 pass(es), 1,385 fail(s), and 13,510 exception(s). View

Hmmm. Patch doesn't look right.

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

Status: Needs review » Needs work

The last submitted patch, 28: 2278383-drupal-set-message-28.patch, failed testing.

Xano’s picture

I am still against using the trait as it is of limited usefulness and does not help with setting up proper dependency injection. Not just services, but also classes that have some sort of container-based factory method shouldn't use this trait. Which use cases would then be left? StringTranslationTrait is an exception, because the particular t() method is necessary for translation extraction.

The last submitted patch, 28: 2278383-drupal-set-message-28.patch, failed testing.

Xano’s picture

Assigned: twistor » Xano
Crell’s picture

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...)

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
13.3 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,216 pass(es), 14 fail(s), and 0 exception(s). View
8.11 KB

Trait be gone! (whoosh)

I also fixed an issue merging in the autoescape patch.

Status: Needs review » Needs work

The last submitted patch, 34: 2278383-drupal-set-message-34.patch, failed testing.

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
13.79 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,192 pass(es). View
2.28 KB

Fixes test failures.

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

Xano’s picture

I assigned the issue to myself in #32 and was just about to upload a new patch. I'm not angry about this, but please pay attention to the assigned issue property in order to prevent duplicate efforts.

Fabianx’s picture

Status: Needs review » Needs work
+++ 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.

kim.pepper’s picture

Sorry Xano :-( All yours.

Xano’s picture

Assigned: Xano » Unassigned
Status: Needs work » Needs review
FileSize
614 bytes
13.91 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,231 pass(es). View

I added a few missing code coverage annotations.

  1. +++ b/core/includes/bootstrap.inc
    @@ -962,9 +952,22 @@ function drupal_set_message($message = NULL, $type = 'status', $repeat = FALSE)
    +  // Workaround for Drush.
    +  if (!\Drupal::hasService('messenger')) {
    +    return array();
    +  }
    

    Shouldn't this workaround be in Drush instead? We generally don't cater for specific contrib cases in core.

  2. +++ b/core/lib/Drupal/Core/Messenger/Messenger.php
    @@ -0,0 +1,68 @@
    +    if (!isset($_SESSION)) {
    +      $_SESSION = array();
    +    }
    

    What does this solve? $_SESSION should exist once the session has been started. If the session hasn't been started at this point in the code, something went wrong.

  3. +++ b/core/lib/Drupal/Core/Messenger/Messenger.php
    @@ -0,0 +1,68 @@
    +        'safe' => SafeMarkup::isSafe($message),
    +        'message' => $message,
    

    This breaks the documentation of the interface, as the outputa of getMessages() and getMessagesByType() are no longer arrays of strings. 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.

Sorry Xano :-( All yours.

No worries. I just wanted to prevent accidental duplicate efforts.

Fabianx’s picture

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

Fabianx’s picture

Status: Needs review » Needs work
Xano’s picture

@kim.pepper: Could you answer questions 1 and 2 from #40? I'm not sure about those two things.

kim.pepper’s picture

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

Xano’s picture

Status: Needs work » Needs review
FileSize
18.45 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View

Status: Needs review » Needs work

The last submitted patch, 45: drupal_2278383_45.patch, failed testing.

Fabianx’s picture

This looks great now! Thanks so much!

tim.plunkett’s picture

+++ 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...

Xano’s picture

Status: Needs work » Needs review
FileSize
2.44 KB
Xano’s picture

FileSize
18.22 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View

Status: Needs review » Needs work

The last submitted patch, 50: drupal_2278383_49.patch, failed testing.

kim.pepper’s picture

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

Xano’s picture

Drush can easily be patched to check for the service's existence. However, apparently we still have code in core that calls drupal_set_message() or drupal_get_message() when the service does not exist yet. I wonder if we should debug this, or just add back the hack from the previous patch until all code has been converted to use the service rather than the functions. I'm leaning towards the latter.

kim.pepper’s picture

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.

Xano’s picture

Status: Needs work » Needs review
FileSize
665 bytes
18.35 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,734 pass(es), 0 fail(s), and 16 exception(s). View

I added @twistor's original check for the messenger's exstistence back in, but without mentioning Drush directly.

Status: Needs review » Needs work

The last submitted patch, 55: drupal_2278383_55.patch, failed testing.

Xano’s picture

Status: Needs work » Needs review
FileSize
3.29 KB
18.77 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,335 pass(es), 386 fail(s), and 25 exception(s). View

Status: Needs review » Needs work

The last submitted patch, 57: drupal_2278383_57.patch, failed testing.

Xano’s picture

Status: Needs work » Needs review
FileSize
3.44 KB
19.24 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,340 pass(es), 385 fail(s), and 25 exception(s). View

Status: Needs review » Needs work

The last submitted patch, 59: drupal_2278383_59.patch, failed testing.

Xano’s picture

Status: Needs work » Needs review
FileSize
7.09 KB
26.34 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh --clean. View

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.

Status: Needs review » Needs work

The last submitted patch, 61: drupal_2278383_61.patch, failed testing.

Xano’s picture

Status: Needs work » Needs review
FileSize
808 bytes
26.34 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View

Status: Needs review » Needs work

The last submitted patch, 63: drupal_2278383_63.patch, failed testing.

Xano’s picture

Status: Needs work » Needs review
FileSize
822 bytes
26.35 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,794 pass(es). View
kim.pepper’s picture

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

Fabianx’s picture

Status: Needs review » Needs work
+++ 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!

Crell’s picture

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?

znerol’s picture

Xano’s picture

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

It is used in the installer. See the last interdiff.

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.

Yeah. The thing is that almost every method needs to work with a different storage variable: either $_SESSION or $this->messages.

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?

I was told off for wanting to introduce a new Symfony component in Austin, so I'm not sure we can do that here. We will definitely need our own interface that extends the Symfony one, though, as theirs does not allow preventing repeated messages.

Crell’s picture

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.

Fabianx’s picture

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.

Xano’s picture

I don't like FlashBagInterface, because the methods are not very self-descriptive and the code documentation is very bare. On top of that, it misses the repeat flag.

Lets fix the nitpicks

Could you reply to my replies to your nitpicks? :-P

Fabianx’s picture

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.

Xano’s picture

Status: Needs work » Needs review
FileSize
26.36 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,027 pass(es). View

Re-roll.

Xano’s picture

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.

It's not a method, but a property, and the service ID is page_cache_kill_switch.

Fabianx’s picture

Status: Needs review » Needs work
+++ 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.

Xano’s picture

Status: Needs work » Needs review
FileSize
1.13 KB
26.36 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,035 pass(es). View

Thanks for clarifying that! I fixed the property description and changed the three-space indentation to two spaces.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC then, thanks so much

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

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.

dawehner’s picture

Status: Needs work » Needs review

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

effulgentsia’s picture

Version: 8.0.x-dev » 8.1.x-dev
Status: Needs review » Postponed
tibbsa’s picture

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

dawehner’s picture

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.

Crell’s picture

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.

Fabianx’s picture

Issue tags: +Needs beta evaluation
Xano’s picture

I understand and agree with the decision that we need a period of only fixing bugs before we publish a new Drupal version, but that period does not need to be 6+ months. This also does not add new features and things like testability should have a very high priority, as they support improved code quality.

Next to that, deciding that these internal refactorings should be postponed long before the aforementioned bug-fix-only period has started is a huge demotivation to people who are willing to spend their time on improving the code base *and* to people who are interested in adopting Drupal.

TLDR; I vote for re-opening this issue.

kim.pepper’s picture

Issue summary: View changes

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() !)

kim.pepper’s picture

Issue summary: View changes
kim.pepper’s picture

Title: Convert drupal_set_message() to a service » Create an injectible service for drupal_set_message()

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

kim.pepper’s picture

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.

Xano’s picture

If I follow the beta changes flow chart, I come to the conclusion that this could possibly be allowed because it reduces fragility, as we will no longer need to declare drupal_set_message() in every unit test, something that has broken test runs in the past.

jibran’s picture

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

kim.pepper’s picture

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

dawehner’s picture

Version: 8.1.x-dev » 8.0.x-dev
Status: Postponed » Active

I don't have any problem with doing that.

Fabianx’s picture

Issue tags: -Needs beta evaluation
alexpott’s picture

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.

xjm’s picture

Version: 8.0.x-dev » 8.1.x-dev
Status: Active » Postponed

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!

alexpott’s picture

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.

dawehner’s picture

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.

Pol’s picture

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

Fabianx’s picture

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.

hass’s picture

Mile23’s picture

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.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

andypost’s picture

Status: Postponed » Active
kim.pepper’s picture

dawehner’s picture

Status: Active » Needs review
FileSize
22.46 KB

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

Status: Needs review » Needs work

The last submitted patch, 108: 2278383-108.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
22.28 KB

Let's see whether this helps

Status: Needs review » Needs work

The last submitted patch, 110: 2278383-110.patch, failed testing.

xjm’s picture

dawehner’s picture

Status: Needs work » Needs review
FileSize
22.55 KB
1.72 KB

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

Status: Needs review » Needs work

The last submitted patch, 113: 2278383-113.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
21.88 KB
3.21 KB

Less failures

jibran’s picture

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

dawehner’s picture

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.

dawehner’s picture

@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.

jibran’s picture

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?

Fabianx’s picture

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

dawehner’s picture

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.

jibran’s picture

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?

Xano’s picture

Status: Needs review » Needs work
  1. +++ b/core/lib/Drupal/Core/EventSubscriber/MaintenanceModeSubscriber.php
    @@ -73,14 +81,17 @@ class MaintenanceModeSubscriber implements EventSubscriberInterface {
         $this->stringTranslation = $translation;
         $this->urlGenerator = $url_generator;
         $this->account = $account;
         $this->bareHtmlPageRenderer = $bare_html_page_renderer;
    +    $this->messenger = $messenger;
       }
     
       /**
    @@ -118,10 +129,10 @@ public function onKernelRequestMaintenance(GetResponseEvent $event) {
    
    @@ -118,10 +129,10 @@ public function onKernelRequestMaintenance(GetResponseEvent $event) {
             // settings page.
             if ($route_match->getRouteName() != 'system.site_maintenance_mode') {
               if ($this->account->hasPermission('administer site configuration')) {
    -            $this->drupalSetMessage($this->t('Operating in maintenance mode. <a href=":url">Go online.</a>', array(':url' => $this->urlGenerator->generate('system.site_maintenance_mode'))), 'status', FALSE);
    +            $this->messenger->addMessage($this->t('Operating in maintenance mode. <a href=":url">Go online.</a>', [':url' => $this->urlGenerator->generate('system.site_maintenance_mode')]), 'status', FALSE);
    
    +++ b/core/lib/Drupal/Core/Messenger/MessengerInterface.php
    @@ -0,0 +1,82 @@
    + * Stores messages send out to the user on the page.
    

    s/send/sent

  2. +++ b/core/lib/Drupal/Core/Messenger/MessengerInterface.php
    @@ -0,0 +1,82 @@
    + * Examples for these messages are for example that a specific content got
    

    @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.

  3. +++ b/core/lib/Drupal/Core/Messenger/MessengerInterface.php
    @@ -0,0 +1,82 @@
    +  const STATUS = 'status';
    

    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.

  4. +++ b/core/lib/Drupal/Core/Messenger/MessengerInterface.php
    @@ -0,0 +1,82 @@
    +   * @param string $message
    

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

  5. +++ b/core/lib/Drupal/Core/Messenger/MessengerInterface.php
    @@ -0,0 +1,82 @@
    +   *   (optional) The message's type. Either self::STATUS, self::WARNING, or
    

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

  6. +++ b/core/lib/Drupal/Core/Messenger/MessengerInterface.php
    @@ -0,0 +1,82 @@
    +   * @return $this
    

    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?

  7. +++ b/core/lib/Drupal/Core/Messenger/MessengerInterface.php
    @@ -0,0 +1,82 @@
    +   *   The messages' type. Either self::STATUS, self::WARNING, or self::ERROR.
    

    We could update the constant names to self::TYPE_* here too.

  8. +++ b/core/lib/Drupal/Core/Messenger/SessionMessenger.php
    @@ -0,0 +1,97 @@
    + * This is the default implementation used by Drupal to store message to the
    

    The class does not know it's the default implementation, and shouldn't say it is. That's all up to the service container.

  9. +++ b/core/lib/Drupal/Core/Messenger/SessionMessenger.php
    @@ -0,0 +1,97 @@
    +    // MarkupInterface objects are detected.
    

    Do we support MarkupInterface or TranslatableMarkup?

  10. +++ b/core/lib/Drupal/Core/Messenger/StaticMessenger.php
    @@ -0,0 +1,104 @@
    +   *   - message (string): the message.
    

    string|\Drupal\Core\StringTranslation\TranslatableMarkup

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

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

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

chx’s picture

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

jibran’s picture

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.

Fabianx’s picture

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

Xano’s picture

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

There are some factual concerns, but I agree that personal preference and experience influence how serious those concerns are to people. I don't mean to bikeshed anything, but since this is about a new public API, it's part of the discussion here.
Personally I have not been able to think of reasons for introducing a fluent interface here. It's not some kind of builder we are adding, so repeated calls to the same instance will be rare, if they ever happen at all.
I quickly checked #2134513: [policy, no patch] Trait strategy and I did not see anything relevant. Could you perhaps point us to the correct comment?

dawehner’s picture

Status: Needs work » Needs review

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

dawehner’s picture

Here is the patch as well.

Wim Leers’s picture

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: 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.

Fabianx’s picture

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

dawehner’s picture

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

Xano’s picture

The reason I raised this, is because I figured the newly introduced public API (the interface) might depend on whether or not we support cacheability data. If it does indeed depend on it, it's in scope and MUST be fixed in this patch, because we cannot break BC once this new service has been released into the wild as part of a stable Drupal release. If adding cacheability metadata is not and will not be relevant, then let's just keep going the way we were and get this thing in :)

On second thought: @Wim Leers is obviously right in #130 where he said that messages have a max-age of 0, since they can never be cached at all. Based on that we do not have to provide any caching functionality in the newly introduced public interface.

Xano’s picture

Status: Needs review » Needs work

@dawehner: Thanks for the new patch, and @Wim Leers thanks for reviewing this from a cacheability point of view!

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

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

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.

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 don't really mind, except that this is hard to update in the future. Nowhere near a blocker, though.

dawehner’s picture

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 :)

dawehner’s picture

Status: Needs work » Needs review

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

dawehner’s picture

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

Xano’s picture

Thanks for clarifying the types!

+++ b/core/lib/Drupal/Core/Messenger/MessengerInterface.php
@@ -45,7 +45,7 @@ public function addMessage($message, $type = self::TYPE_STATUS, $repeat = FALSE)
+   * @return string[][]|\Drupal\Component\Render\MarkupInterface[][]

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

Xano’s picture

Correcting what looks like accidentally incorrect commit credit settings.

dawehner’s picture

dawehner’s picture

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.

Gábor Hojtsy’s picture

Fabianx’s picture

Xano’s picture

Status: Needs review » Reviewed & tested by the community

@dawehner: Agreed. Let's go for it.

I went through the patch again and this looks good to go to me. Thanks, everybody!

xjm’s picture

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.

catch’s picture

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

catch’s picture

Status: Reviewed & tested by the community » Needs review
catch’s picture

Status: Needs review » Needs work

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

joelpittet’s picture

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

andypost’s picture

Status: Needs review » Reviewed & tested by the community

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

alexpott’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs framework manager review

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!

  • alexpott committed ee22272 on 8.2.x
    Issue #2278383 by Xano, dawehner, kim.pepper, twistor, joelpittet,...
claudiu.cristea’s picture

Issue tags: +Needs change record

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

Fabianx’s picture

#153 A change record should be good indeed.

dawehner’s picture

I'' write one

dawehner’s picture

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

  • alexpott committed 21a4b87 on 8.2.x
    Revert "Issue #2278383 by Xano, dawehner, kim.pepper, twistor,...
alexpott’s picture

Status: Fixed » Needs work
+++ 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.

alexpott’s picture

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

twistor’s picture

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

Can't we just fix Drush?

alexpott’s picture

@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.

alexpott’s picture

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

dawehner’s picture

catch’s picture

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 [];

  • alexpott committed 21a4b87 on 8.3.x
    Revert "Issue #2278383 by Xano, dawehner, kim.pepper, twistor,...
  • alexpott committed ee22272 on 8.3.x
    Issue #2278383 by Xano, dawehner, kim.pepper, twistor, joelpittet,...

  • alexpott committed 21a4b87 on 8.3.x
    Revert "Issue #2278383 by Xano, dawehner, kim.pepper, twistor,...
  • alexpott committed ee22272 on 8.3.x
    Issue #2278383 by Xano, dawehner, kim.pepper, twistor, joelpittet,...
dawehner’s picture

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.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Mile23’s picture

catch’s picture

@dawehner I'm not @alexpott but assuming there's an exception when you try to install with the patch applied via drush - that seems like enough reason to roll back? Either we'd need to add back in the workaround, or get drush fixed upstream (but even then individual installs will still be out of date).

dawehner’s picture

Thank you for answering my question!

Okay, so adding this layer is enough here?

catch’s picture

Yeah I think it's https://www.drupal.org/node/2278383#comment-11429167 reverting the interdiff but then adding a longer comment about why it's there.

Also it's not a bc layer at the moment, it's just eating failure - it should probably clear the messages if $clear = TRUE?

jibran’s picture

So we just need to address #172.

Fabianx’s picture

I would still suggest (if getting this in poses to be difficult) a 2/3-step process to get this in:

- Rename drupal_set_message to _drupal_set_message - instead of removing, mark as @internal and possibly @deprecated
- Have drupal_set_message call \Drupal::service('messenger') (etc. - like in this patch)
- Create a LegacyMessenger class that calls the _ functions
(e.g. possibly similar to http://cgit.drupalcode.org/service_container/tree/src/Messenger/LegacyMe... though also just calling the functions is fine with me.)

Then in a 2nd follow-up issue do the actual conversion and deal with BC, etc.

The advantage of splitting design / interface and implementation is that we can have the new interfaces and services, but don't have to worry about BC and breaking code until later.

--

Just a suggestion to get the important part (the service and the interface) finally in.

dawehner’s picture

I really like this approach. This allows us to not focus on the failure for now. Great suggestion @Fabianx!

Pol’s picture

Me like it too.

jakewise93@gmail.com’s picture

You would be better off separating the responsibilities of the messenger service both handing notifications, and notifying page_cache_kill_switch service, by dispatching an event instead. You could then have an observer that is responsible for triggering the page_cache_kill_switch when the messenger service is triggered.

Also I think the API could do with being modernised a bit, and removing the need for constants. I would take some tips from PSR's LoggerInterface (https://github.com/php-fig/log/blob/master/Psr/Log/LoggerInterface.php) and have something along the lines of:

interface MessengerInterface {
  public function sendStatus($message, $repeat = FALSE);
  public function sendWarning($message, $repeat = FALSE);
  public function sendError($message, $repeat = FALSE);
  ...
}

That way you can be sure that the concrete implementation handles each expected type of message properly. As the BC layer I would then suggest that the implementation of drupal_set_message() be changed to:

function drupal_set_message($message = NULL, $type = 'status', $repeat = FALSE) {
  /** @var MessengerInterface $messenger */
  $messenger = \Drupal::service('messenger');
  switch ($type) {
    case 'status':
      $messenger->sendStatus($message, $repeat);
      break;
    case 'warning':
      $messenger->sendWarning($message, $repeat);
      break;
    case 'error':
      $messenger->sendError($message, $repeat);
      break;
  }
}
dawehner’s picture

The comment in #177 is why we should things one step at a time and then discuss that later.

Fabianx’s picture

+1 to #177: Lets create a nice interface first :).

dawehner’s picture

To be honest the interface as of now as part of the patch seems to be reasonable for me.

  • alexpott committed 21a4b87 on 8.4.x
    Revert "Issue #2278383 by Xano, dawehner, kim.pepper, twistor,...
  • alexpott committed ee22272 on 8.4.x
    Issue #2278383 by Xano, dawehner, kim.pepper, twistor, joelpittet,...

  • alexpott committed 21a4b87 on 8.4.x
    Revert "Issue #2278383 by Xano, dawehner, kim.pepper, twistor,...
  • alexpott committed ee22272 on 8.4.x
    Issue #2278383 by Xano, dawehner, kim.pepper, twistor, joelpittet,...

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

jibran’s picture

Issue tags: +Needs reroll
Jo Fitzgerald’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
22.71 KB

Re-rolled.

jibran’s picture

This addresses #158 and we are back to RTBC because we don't have any other feedback. I marked #2775211: [PP-1] Test and document why drupal_set_message() has a workaround for a missing container and #2760167: Add \Drupal\Core\Messenger\Messenger as postpone on this.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 186: create_an_injectible-2278383-186.patch, failed testing. View results

jibran’s picture

Status: Needs work » Needs review
FileSize
579 bytes
22.83 KB

PHPCS fix.

Status: Needs review » Needs work

The last submitted patch, 188: create_an_injectible-2278383-188.patch, failed testing. View results

jibran’s picture

Status: Needs work » Needs review
FileSize
1.33 KB
22.78 KB

Hopefully, this will fix the fails.

dawehner’s picture

Some changes:

What do you think?

Status: Needs review » Needs work

The last submitted patch, 191: 2278383-191.patch, failed testing. View results

dawehner’s picture

Status: Needs work » Needs review
FileSize
19.03 KB
1.4 KB

This fixes a couple of the installation issues ...

Status: Needs review » Needs work

The last submitted patch, 193: 2278383-193.patch, failed testing. View results

dawehner’s picture

Status: Needs work » Needs review
FileSize
15.93 KB
0 bytes

Let's remove the unit test for the class, which no longer exists.

Wim Leers’s picture

For now, a superficial review, with first impressions.

  1. +++ b/core/core.services.yml
    @@ -1659,3 +1659,6 @@ services:
    +  messenger:
    

    Totally off-topic: I can't see this and not think of Facebook Messenger or MSN Messenger :P

  2. +++ b/core/includes/bootstrap.inc
    @@ -457,30 +456,21 @@ function watchdog_exception($type, Exception $exception, $message = NULL, $varia
    +  // Workaround for code that can not check if the service exists.
    

    Hm I'm not sure what this means exactly…

  3. +++ b/core/lib/Drupal/Core/Messenger/LegacyMessenger.php
    @@ -0,0 +1,153 @@
    +/**
    + * The old implementation of drupal_get_messages
    + *
    + * /**
    

    This seems wrong: * /**.

  4. +++ b/core/lib/Drupal/Core/Messenger/MessengerInterface.php
    @@ -0,0 +1,125 @@
    +   * Adds a new message to the queue.
    

    It's a queue! This is important to state in the docblock, I think: that messages are presented to the user in the order they were added to the queue. And therefore the message order depends on the code execution order. No sorting.

  5. +++ b/core/lib/Drupal/Core/Messenger/MessengerInterface.php
    @@ -0,0 +1,125 @@
    +   * Gets all messages.
    ...
    +  public function getMessages();
    

    Note: Symfony uses all() for this. Maybe we should too?

dawehner’s picture

Note: Symfony uses all() for this. Maybe we should too?

This is a good point.
I tried to rename a good amount of methods now:

  • getMessages() -> all()
  • getMessagesByType() -> messagesByType() (Note: I'm not particulary happy about this choice)
  • deleteMessages() -> deleteAll()
  • deleteMessagesByType() -> deleteByType()

Status: Needs review » Needs work

The last submitted patch, 197: 2278383-197.patch, failed testing. View results

dawehner’s picture

Status: Needs work » Needs review
FileSize
15.92 KB
488 bytes

Let's fix it :)

kim.pepper’s picture

Thanks @dawehner!

A couple of minor tweaks:

  1. +++ b/core/includes/bootstrap.inc
    @@ -457,30 +456,22 @@ function watchdog_exception($type, Exception $exception, $message = NULL, $varia
    +  // This function might be called in the early installer, so we check for now
    +  // whether the service exists.
    

    This wording can be improved. What about: "This function might be called early on in the installer, so we check whether the service exists."

  2. +++ b/core/lib/Drupal/Core/Messenger/LegacyMessenger.php
    @@ -0,0 +1,150 @@
    + * @deprecated Deprecated as of Drupal 8.2.
    

    8.4?

pritish.kumar’s picture

Made the changes as specified in #200.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

jibran’s picture

Status: Needs review » Reviewed & tested by the community

The patch looks good to me. We should update change notice and add all new methods to it.

Mile23’s picture

Status: Reviewed & tested by the community » Needs work

Poor dsm(). Always one day too late to be refactored. Can we please move this to 8.4.x in some way, or is it too much of an API change?

  1. +++ b/core/includes/bootstrap.inc
    @@ -507,12 +498,17 @@ function drupal_set_message($message = NULL, $type = 'status', $repeat = FALSE)
    + * @deprecated Deprecated as of Drupal 8.4.
    

    Say '@deprecated in Drupal 8.4.x, will be removed before Drupal 9.0.0.'

    Unless we're really going to wait for 8.5.x on this.

    Also needs a follow-up to @trigger_error() on this.

  2. +++ b/core/includes/bootstrap.inc
    @@ -507,12 +498,17 @@ function drupal_set_message($message = NULL, $type = 'status', $repeat = FALSE)
    @@ -520,7 +516,7 @@ function drupal_get_messages($type = NULL, $clear_queue = TRUE) {
    

    Should drupal_get_messages() also be deprecated?

kim.pepper’s picture

Updated change notice.

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
15.96 KB
1 KB

Unless we're really going to wait for 8.5.x on this.

That ship has sailed.

Updated the deprecation message.

jibran’s picture

kim.pepper’s picture

A few minor nitpicks which can be fix on commit.

  1. +++ b/core/lib/Drupal/Core/Messenger/LegacyMessenger.php
    @@ -0,0 +1,150 @@
    + * @deprecated Deprecated as of Drupal 8.4.
    

    Reference to 8.4

  2. +++ b/core/lib/Drupal/Core/Messenger/LegacyMessenger.php
    @@ -0,0 +1,150 @@
    + * @deprecated Deprecated as of Drupal 8.4.
    

    Another reference to 8.4

  3. +++ b/core/lib/Drupal/Core/Messenger/MessengerInterface.php
    @@ -0,0 +1,127 @@
    + * An example for these messages is for example: "Content X got saved".
    

    Needs a grammar fix.

larowlan’s picture

Status: Reviewed & tested by the community » Needs review
  1. +++ b/core/includes/bootstrap.inc
    @@ -507,12 +498,17 @@ function drupal_set_message($message = NULL, $type = 'status', $repeat = FALSE)
    +  if (($messenger = \Drupal::hasService('messenger') ? \Drupal::service('messenger') : NULL) && $messages = $messenger->all()) {
    

    This is quite a convoluted ternary. Can we add brackets around it, or break it onto it's own line please?

  2. +++ b/core/lib/Drupal/Core/Messenger/LegacyMessenger.php
    @@ -0,0 +1,150 @@
    +function _drupal_set_message($message = NULL, $type = 'status', $repeat = FALSE) {
    ...
    +function _drupal_get_messages($type = NULL, $clear_queue = TRUE) {
    

    Any reason why these are stand alone and not just part of the LegacyMessenger class?

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

    If we can't be certain that the messenger service exists (this legacy class is used in early bootstrap etc) - can we be sure the page cache kill switch exists? If so then is it being added by the Installer service provider? If so any reason why we can't do the same?

larowlan’s picture

larowlan’s picture

In regards to my questions 2 and 3, saw earlier comments from @FabianX and the follow-ups.

However, I'm not totally convinced on the stand alone functions, I think that inlining them into the class as protected methods would avoid needing to have them marked as deprecated/internal.

Happy to punt the service container workaround and use of SESSION to the follow up.

dawehner’s picture

This is quite a convoluted ternary. Can we add brackets around it, or break it onto it's own line please?

Fair, I tried to fix it.

However, I'm not totally convinced on the stand alone functions, I think that inlining them into the class as protected methods would avoid needing to have them marked as deprecated/internal.

I think a good middleground are private functions on the object, aka. functions which can be completely changed in the future.

Status: Needs review » Needs work

The last submitted patch, 212: 2278383-212.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Jo Fitzgerald’s picture

Status: Needs work » Needs review
FileSize
16.07 KB
1.11 KB

Moved ternary onto it's own line.
Removed redundant newline.

dawehner’s picture

Thank you @Jo Fitzgerald!

jibran’s picture

Status: Needs review » Needs work

#209.1 and #209.2 are addressed just two minor issuse.

  1. +++ b/core/lib/Drupal/Core/Messenger/LegacyMessenger.php
    @@ -0,0 +1,150 @@
    +   * @internal ¶
    

    It'd be nice to add some docs here.

  2. +++ b/core/lib/Drupal/Core/Messenger/LegacyMessenger.php
    @@ -0,0 +1,150 @@
    +   * @deprecated Deprecated as of Drupal 8.5.
    

    s/8.5/8.5.x

dawehner’s picture

Status: Needs work » Needs review
FileSize
17.78 KB
2.73 KB

Thank you @jibran

Status: Needs review » Needs work

The last submitted patch, 217: 2278383-217.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Jo Fitzgerald’s picture

@dawehner It looks like you based your patch off #212 rather than #214 - you have re-introduced your error around the ternary in bootstrap.php.

dawehner’s picture

Status: Needs work » Needs review
FileSize
17.78 KB

Oh you are totally right. Thank you!

jibran’s picture

Status: Needs review » Reviewed & tested by the community

Thanks, @dawehner and @Jo Fitzgerald. This is ready.

larowlan’s picture

Status: Reviewed & tested by the community » Needs review
  1. +++ b/core/includes/bootstrap.inc
    @@ -457,30 +456,22 @@ function watchdog_exception($type, Exception $exception, $message = NULL, $varia
    +  // This function might be called early on in the installer, so we check
    +  // whether the service exists.
    ...
    +  if (\Drupal::hasService('messenger')) {
    ...
    +    $messenger = new LegacyMessenger();
    
    +++ b/core/lib/Drupal/Core/Messenger/LegacyMessenger.php
    @@ -0,0 +1,195 @@
    +      \Drupal::service('page_cache_kill_switch')->trigger();
    

    If we're not safe to universally call \Drupal::service('messener') are there instances where unconditionally calling for the page cache kill switch are problematic too?

  2. +++ b/core/includes/bootstrap.inc
    @@ -507,12 +498,18 @@ function drupal_set_message($message = NULL, $type = 'status', $repeat = FALSE)
    + * @deprecated in Drupal 8.5.0, will be removed before Drupal 9.0.0.
    + *   Use \Drupal::service('messenger')->getMessages() or
    + *   \Drupal::service('messenger')->getMessagesByType() instead.
    

    I think this is supposed to link to the change record?
    See https://www.drupal.org/core/deprecation

  3. +++ b/core/lib/Drupal/Core/Messenger/LegacyMessenger.php
    @@ -0,0 +1,195 @@
    +   * @internal ¶
    

    nit: trailing whitespace, can be fixed on commit

kim.pepper’s picture

  1. I wrapped the page_cache_kill_switch in a service check.
  2. Fixed
  3. Fixed
kim.pepper’s picture

Discussed with @larowlan in chat whether or not we still need to check if the messenger service exists in the early install phase, and decided to post a patch here that removes it to see if tests pass. It was 3 years ago this was a problem, after all.

(interdiff is against #220)

larowlan’s picture

One minor nit for 224 (if it should pass)

+ * (optional) The page cache kill switch.

its not optional anymore

can be fixed on commit

kim.pepper’s picture

If tests are green, we need to do a site install with a recent version of drush.

kim.pepper’s picture

Manually did a site install using drush 9.0.0-beta4 and did not see an error. \m/

jibran’s picture

Status: Needs review » Reviewed & tested by the community

#226 was my only concern after the changes we made in #224. I think patch is ready. We don't need to test it with 8.4.x beacuse we are not going to backport it so RTBC.

larowlan’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/includes/bootstrap.inc
@@ -457,30 +456,15 @@ function watchdog_exception($type, Exception $exception, $message = NULL, $varia
 function drupal_set_message($message = NULL, $type = 'status', $repeat = FALSE) {

+++ b/core/lib/Drupal/Core/Messenger/LegacyMessenger.php
@@ -0,0 +1,213 @@
+ * @deprecated Deprecated as of Drupal 8.5.x

Sorry, this needs a @see to the change record too.

Do we need to be adding trigger_error here too?

Or is that only after everything is converted? I will try and find clarification.

larowlan’s picture

+++ b/core/includes/bootstrap.inc
@@ -457,30 +456,15 @@ function watchdog_exception($type, Exception $exception, $message = NULL, $varia
+ * @deprecated in Drupal 8.5.0, will be removed before Drupal 9.0.0.
+ *   Use \Drupal::service('messenger')->addMessage() instead.

@@ -507,12 +491,20 @@ function drupal_set_message($message = NULL, $type = 'status', $repeat = FALSE)
+ * @deprecated in Drupal 8.5.0, will be removed before Drupal 9.0.0.
+ *   Use \Drupal::service('messenger')->getMessages() or
+ *   \Drupal::service('messenger')->getMessagesByType() instead.
+ *
+ * @see https://www.drupal.org/node/2774931

+++ b/core/lib/Drupal/Core/Messenger/LegacyMessenger.php
@@ -0,0 +1,213 @@
+ * @deprecated Deprecated as of Drupal 8.5.x

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

Discussed with @catch and we agreed on the following plan

- remove the deprecation of drupal_set_message/drupal_get_message and the LegacyMessenger.
- mark MessengerInterface internal (for now)
- keep LegacyMessenger as internal

Then in #2760167: Add \Drupal\Core\Messenger\Messenger when we have a genuine alternative, add the deprecations back, and remove internal from MessengerInterface

No need for trigger_error here either, that will come in the follow-up when we do have a non deprecated alternative

dawehner’s picture

I see, so we keep things as internal as possible till we figure out some way to deal with it properly.

@kim.pepper If you have time, please give it a try.

kim.pepper’s picture

Sounds good.

jibran’s picture

Status: Needs review » Reviewed & tested by the community

#230 is a good plan. Thanks for the changes @kim.pepper

  • larowlan committed 48a8e53 on 8.5.x
    Issue #2278383 by dawehner, Xano, kim.pepper, twistor, jibran, Jo...
larowlan’s picture

Status: Reviewed & tested by the community » Fixed

Credited @catch for time yesterday debating/devising the BC/deprecation approach that we settled on.

fixed on commit

diff --git a/core/includes/bootstrap.inc b/core/includes/bootstrap.inc
index f8b6228..5a1cfbe 100644
--- a/core/includes/bootstrap.inc
+++ b/core/includes/bootstrap.inc
@@ -11,7 +11,6 @@
 use Drupal\Component\Utility\Unicode;
 use Drupal\Core\Config\BootstrapConfigStorageFactory;
 use Drupal\Core\Logger\RfcLogLevel;
-use Drupal\Core\Messenger\LegacyMessenger;
 use Drupal\Core\Test\TestDatabase;
 use Drupal\Core\Session\AccountInterface;
 use Drupal\Core\Site\Settings;

Committed as 48a8e53 and pushed to 8.5.x.

Unpostponed the followup #2760167: Add \Drupal\Core\Messenger\Messenger and updated the issue summary over there.

Would be good to see some movement over there so we can deprecate drupal_set_message and remove the LegacyMessenger in time for 8.5.0-alpha1.

larowlan’s picture

Moved the change record to the follow-up as its going to be when we do the actual deprecation.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.