Problem/Motivation

#2278383: Create an injectible service for drupal_set_message() was added in a preemptive move to convert the proceedural functions drupal_set_message() and drupal_get_messages() into a dedicated "messenger" service.

Even though this was added, it was done so with full knowledge that it wasn't yet finalized and more work needed to be done to make this a stable service.

Once this issue is in, #2924538: [META] Remove all usages of drupal_get_message and drupal_set_message needs to be tackled.

Proposed resolution

  • Create a stable Messenger service using Symfony's FlashBag service.
  • Refactor the existing \Drupal\Core\Messenger\LegacyMessenger implementation to provide backwards compatibility that will ultimately be removed in 9.x.
  • Create the \Drupal::messenger() helper method to be used in procedural functions.
  • Deprecate drupal_set_message(), drupal_get_messages() and <code>\Drupal\Core\Messenger\LegacyMessenger - triggering deprecation errors as needed

Remaining tasks

  • Create patch
  • Create tests

User interface changes

None

API changes

The "messenger" service now uses <code>\Drupal\Core\Messenger\Messenger instead of \Drupal\Core\Messenger\LegacyMessenger (which is now deprecated).

Data model changes

None

CommentFileSizeAuthor
#175 2760167-173-175-interdiff.txt612 bytesmarkcarver
#175 2760167-175.patch28.92 KBmarkcarver
#173 2760167-167-172-interdiff.txt4.7 KBmarkcarver
#173 2760167-171.patch28.91 KBmarkcarver
#172 2760167-172-interdiff.txt2.66 KBkim.pepper
#172 2760167-172.patch27.45 KBkim.pepper
#167 2760167-162-167-interdiff.txt2.31 KBmarkcarver
#167 2760167-167.patch28.32 KBmarkcarver
#162 2760167-161-162-interdiff.txt708 bytesmarkcarver
#162 2760167-162.patch27.96 KBmarkcarver
#161 2760167-160-161-interdiff.txt834 bytesmarkcarver
#161 2760167-161.patch27.96 KBmarkcarver
#160 2760167-152-160-interdiff.txt1.37 KBmarkcarver
#160 2760167-160.patch27.58 KBmarkcarver
#156 2760167-152.patch27.72 KBmarkcarver
#152 2760167-150-152-interdiff.txt2.04 KBmarkcarver
#152 2760167-152.patch27.72 KBmarkcarver
#150 2760167-146-150-interdiff.txt3.96 KBmarkcarver
#150 2760167-150.patch27.56 KBmarkcarver
#146 2760167-141-146-interdiff.txt481 bytesmarkcarver
#146 2760167-146.patch27.2 KBmarkcarver
#141 2760167-139-141-interdiff.txt9.64 KBmarkcarver
#141 2760167-141.patch27.22 KBmarkcarver
#139 2760167-136-139-interdiff.txt8.37 KBmarkcarver
#139 2760167-139.patch26.16 KBmarkcarver
#136 interdiff-2760167.txt1.62 KBdawehner
#136 2760167-136.patch28.08 KBdawehner
#134 2760167-126-134-interdiff.txt2.32 KBmarkcarver
#134 2760167-134.patch29.6 KBmarkcarver
#126 2760167-125-126-interdiff.txt2.04 KBmarkcarver
#126 2760167-126.patch29.07 KBmarkcarver
#124 2760167-119-124-interdiff.txt17.3 KBmarkcarver
#124 2760167-124.patch29.4 KBmarkcarver
#119 2760167-116-119-interdiff.txt5.61 KBmarkcarver
#119 2760167-119.patch29.2 KBmarkcarver
#116 2760167-110-116-interdiff.txt2.72 KBmarkcarver
#116 2760167-116.patch27.91 KBmarkcarver
#110 2760167-110.patch26.28 KBkim.pepper
#108 2760167-108-interdiff.txt773 byteskim.pepper
#108 2760167-108.patch26.24 KBkim.pepper
#96 2760167-96-interdiff.txt6.05 KBkim.pepper
#96 2760167-96.patch26.28 KBkim.pepper
#92 2760167-92-interdiff.txt7.1 KBkim.pepper
#92 2760167-92.patch25.48 KBkim.pepper
#88 2760167-88-interdiff.txt1.55 KBkim.pepper
#88 2760167-88.patch24.12 KBkim.pepper
#86 2760167-85-interdiff.txt8.81 KBkim.pepper
#86 2760167-85.patch24.14 KBkim.pepper
#79 interdiff-d9c5dd.txt1.24 KBjibran
#75 2760167-75-interdiff.txt9.66 KBkim.pepper
#75 2760167-75.patch20.82 KBkim.pepper
#63 2760167-63-interdiff.txt501 byteskim.pepper
#63 2760167-63-pass.patch17.74 KBkim.pepper
#63 2760167-63-fail.patch16.2 KBkim.pepper
#59 2760167-59-interdiff.txt540 byteskim.pepper
#59 2760167-59-pass.patch17.25 KBkim.pepper
#59 2760167-59-fail.patch15.71 KBkim.pepper
#55 2760167-55-interdiff.txt1.65 KBkim.pepper
#55 2760167-55-pass.patch17.25 KBkim.pepper
#55 2760167-55-fail.patch15.71 KBkim.pepper
#52 2760167-52-interdiff.txt4.17 KBkim.pepper
#52 2760167-52-pass.patch17.21 KBkim.pepper
#52 2760167-52-fail.patch15.67 KBkim.pepper
#48 2760167-48-interdiff.txt1.84 KBkim.pepper
#48 2760167-48-pass.patch17.06 KBkim.pepper
#43 2760167-42-pass.patch17.07 KBkim.pepper
#42 2760167-42-interdiff.txt2.92 KBkim.pepper
#42 2760167-42-fail.patch15.6 KBkim.pepper
#42 2760167-42-pass.patch17.07 KBkim.pepper
#37 2760167-37.patch14.93 KBjibran
#37 interdiff-eed421.txt3.74 KBjibran
#34 2760167-34.patch14.16 KBjibran
#34 interdiff-049a29.txt472 bytesjibran
#32 add-2760167-32.patch14.09 KBjibran
#32 interdiff-7b2ac4.txt604 bytesjibran
#26 2760167-26-PoC.patch2.38 KBWim Leers
#17 interdiff.txt1.92 KBznerol
#17 add-2760167-17.patch14.47 KBznerol
#15 interdiff-2760167.txt1.31 KBdawehner
#11 add-2760167-11.patch13.99 KBjibran
#11 interdiff-5fc868.txt4.67 KBjibran
#9 2760167-9.patch9.96 KBjibran
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

dawehner created an issue. See original summary.

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.

Insasse’s picture

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

dawehner’s picture

Let me quote myself ...

Once #2278383: Create an injectible service for drupal_set_message(): Create an injectible service for drupal_set_message() is in

:)

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

Title: No longer use $_SESSION in \Drupal\Core\Messenger\SessionMessenger » [PP-1] No longer use $_SESSION in \Drupal\Core\Messenger\SessionMessenger
Status: Active » Postponed

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.

larowlan’s picture

Title: [PP-1] No longer use $_SESSION in \Drupal\Core\Messenger\SessionMessenger » No longer use $_SESSION in \Drupal\Core\Messenger\SessionMessenger
Issue summary: View changes
Status: Postponed » Active
jibran’s picture

Title: No longer use $_SESSION in \Drupal\Core\Messenger\SessionMessenger » Add \Drupal\Core\Messenger\Messenger and remove \Drupal\Core\Messenger\LegacyMessenger
Status: Active » Needs review
FileSize
9.96 KB

First try.

Status: Needs review » Needs work

The last submitted patch, 9: 2760167-9.patch, failed testing. View results

jibran’s picture

Status: Needs work » Needs review
FileSize
4.67 KB
13.99 KB

Status: Needs review » Needs work

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

dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/Messenger/Messenger.php
    @@ -0,0 +1,98 @@
    +  public function deleteByType($type) {
    +    return $this->flashBag->get($type);
    +  }
    

    Wow this FlashBagInterface is massively confusing.

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

    Could't you set two types and then remove the first by using the first type? I'm just a bit worried that we accidentally loose some test coverage

jibran’s picture

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

RE: #13

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

FileSize
1.31 KB

I've done some debugging, but I'm not 100% sure yet what is causing it.

This test fires 3 request to Drupal.

1. Front page, from the setup method
2. Front page, from the test method itself
3. Front page, from the test method itself

Note: 2 and 3 are cached by page cache as part of this patch. ... It feels weird that they are actually cached, given that \Drupal\service_provider_test\TestClass sends a message on every request.

Further research shows, that the call to session save in \Drupal\Core\StackMiddleware\Session::handle doens't actually save anything. \Drupal\Core\Session\SessionManager::save calls to \Drupal\Core\Session\SessionManager::isSessionObsolete which returns TRUE in HEAD but FALSE with the patch. Looking at \Drupal\Core\Session\SessionManager::getSessionDataMask it seems to look directly into $_SESSION, which seems to not have any of the messages data.

I thought, okay, we need to maybe look into the messages back properly ... and made changes according to this interdiff ... this didn't worked though, because at that point the messages back no longer has the message as they go printed out already, I think.

Well, this is a bit out over my head right now :) Maybe you can figure it out

jibran’s picture

Assigned: Unassigned » znerol

Time to summon @znerol!

znerol’s picture

Status: Needs work » Needs review
FileSize
14.47 KB
1.92 KB

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

jibran’s picture

WoW it passed.

znerol’s picture

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

FrontPageTest.php:56
info: On front page.
FrontPageTest.php:60
info: On front page.
FrontPageTest.php:67
error: The path '/kittens' is either invalid or you do not have access to it.
FrontPageTest.php:72
error: The path 'node/1' has to start with a slash.
FrontPageTest.php:77
info: The configuration options have been saved.
FrontPageTest.php:80
info: On front page.
FrontPageTest.php:84
info: On front page.

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

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

znerol’s picture

Assigned: znerol » Unassigned

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

Unassigning myself, ideas?

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

jibran’s picture

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

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

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

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

znerol’s picture

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

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

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

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

znerol’s picture

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

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

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

dawehner’s picture

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

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

To understand you 100%, this is the main difference from before and after? Previously, $_SESSION was kept around vs. with the new change, this is no longer the case?

znerol’s picture

Assigned: Unassigned » Wim Leers

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

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

Guess that's a question for @Wim Leers.

Wim Leers’s picture

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

    Nit: s/.././

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

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


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

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

In HEAD:

  1. Any call to drupal_set_message() while running the code that is building a response to the current request would cause PageCache (= for anon users) to not cache the response. But also Dynamic Page Cache (= authenticated users) respects the killswitch:
      page_cache_kill_switch:
        class: Drupal\Core\PageCache\ResponsePolicy\KillSwitch
        tags:
          - { name: page_cache_response_policy }
          - { name: dynamic_page_cache_response_policy }
    
  2. \Drupal\system\Plugin\Block\SystemMessagesBlock simply renders ['#type' => 'status_messages'] and the block itself says it's endlessly cacheable, and it even includes the explanation in its comment:
      public function getCacheMaxAge() {
        // The messages are session-specific and hence aren't cacheable, but the
        // block itself *is* cacheable because it uses a #lazy_builder callback and
        // hence the block has a globally cacheable render array.
        return Cache::PERMANENT;
      }
    
  3. If we then inspect \Drupal\Core\Render\Element\StatusMessages (which corresponds to ['#type' => 'status_messages']), we see that it generates a placeholder unconditionally, i.e. 100% of the time. This means it's perfectly cacheable by Render Cache & Dynamic Page Cache: because there is no variation.
  4. Finally, \Drupal\Core\Render\Element\StatusMessages::renderMessages() is the #lazy_builder callback that runs once on every page. It doesn't return cacheability metadata because it doesn't matter anymore at this point: the #cache key on the render array it returns is not specified because we don't ever need/want rendered status messages to be render cached anyway. And this lazy builder callback is invoked for something that is placeholdered 100% of the time (see previous point.)

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

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

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

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

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

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

jibran’s picture

Assigned: Wim Leers » Unassigned

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

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

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

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

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

Status: Needs review » Needs work

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

jibran’s picture

Title: Add \Drupal\Core\Messenger\Messenger and remove \Drupal\Core\Messenger\LegacyMessenger » [PP-1] Add \Drupal\Core\Messenger\Messenger and remove \Drupal\Core\Messenger\LegacyMessenger
Issue tags: -Needs issue summary update, -Needs title update

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

znerol’s picture

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

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

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

jibran’s picture

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

jibran’s picture

Title: [PP-1] Add \Drupal\Core\Messenger\Messenger and remove \Drupal\Core\Messenger\LegacyMessenger » Add \Drupal\Core\Messenger\Messenger and remove \Drupal\Core\Messenger\LegacyMessenger
Status: Needs work » Needs review
FileSize
604 bytes
14.09 KB

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

kim.pepper’s picture

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

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

jibran’s picture

Status: Needs work » Needs review
FileSize
472 bytes
14.16 KB

Added the docs.

kim.pepper’s picture

Status: Needs review » Reviewed & tested by the community

LGTM

larowlan’s picture

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

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

jibran’s picture

Status: Needs work » Needs review
Issue tags: +DrupalSouth 2017
FileSize
3.74 KB
14.93 KB

Thanks, for the review. Addressed #36.

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

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

kim.pepper’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC

xjm’s picture

Issue summary: View changes

Thanks for working on this.

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

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

jibran’s picture

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

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

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

xjm’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs followup

Thanks @jibran.

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

kim.pepper’s picture

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

kim.pepper’s picture

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

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

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

Status: Needs review » Needs work

The last submitted patch, 43: 2760167-42-pass.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

tim.plunkett’s picture

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

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

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

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

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
17.06 KB
1.84 KB

Ah thanks Tim!

Status: Needs review » Needs work

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

kim.pepper’s picture

Hmm. Pretty sure the message matches now.

tim.plunkett’s picture

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

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

kim.pepper’s picture

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

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

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

kim.pepper’s picture

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

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

kim.pepper’s picture

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

jibran’s picture

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

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

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

jibran’s picture

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

PHPCS error.

kim.pepper’s picture

The last submitted patch, 59: 2760167-59-fail.patch, failed testing. View results

markcarver’s picture

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

One tiny trivial request:

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

dawehner’s picture

I think I agree with @markcarver. Messages are just like a logger also not really worth to unit test, if you ask me.

kim.pepper’s picture

kim.pepper’s picture

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

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

kim.pepper’s picture

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

The last submitted patch, 63: 2760167-63-fail.patch, failed testing. View results

markcarver’s picture

Status: Reviewed & tested by the community » Needs work

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

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

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

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

---

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

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

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

kim.pepper’s picture

Status: Needs work » Needs review

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

This was discussed already in #32

kim.pepper’s picture

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

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

jibran’s picture

Status: Needs review » Reviewed & tested by the community

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

markcarver’s picture

Status: Reviewed & tested by the community » Needs work

This was discussed already in #32

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

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

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

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

MessengerInterface is actually the public facing API, not Messenger.

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

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

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

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

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

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

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

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

xjm’s picture

Above discussion makes me wonder if we should profile this?

xjm’s picture

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

xjm’s picture

Status: Needs work » Needs review

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

kim.pepper’s picture

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

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

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

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

Fixes @xjm comma splices too.

kim.pepper’s picture

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

jibran’s picture

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

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

markcarver’s picture

Status: Needs review » Needs work

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

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

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

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

---

Almost there :D

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

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

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

    This gets a little tricky here I think.

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

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

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

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

---

Can we add tests to replicate drush si fail?

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

jibran’s picture

FileSize
1.24 KB

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

markcarver’s picture

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

jibran’s picture

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

kim.pepper’s picture

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

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

kim.pepper’s picture

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

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

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

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

    // Fallback to memory messenger.
    return $this->memoryMessenger;
  }
?>
markcarver’s picture

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

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

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

came up with the idea of a ChainedMessenger

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

markcarver’s picture

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

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
24.14 KB
8.81 KB

OK here goes.

Added a ChainedMessenger as per above.

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

Again, manually tested locally with drush site-install.

larowlan’s picture

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

    We should mark this as @internal

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

    We should mark this as @internal

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

    Same here

kim.pepper’s picture

jibran’s picture

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

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

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

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

    Let's call it $messengerService.

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

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

markcarver’s picture

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

No. See #71/#78.

markcarver’s picture

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

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

kim.pepper’s picture

Fixes #89 1. and 2.
Fixes #91

Also moved some more duplication into the base class.

Status: Needs review » Needs work

The last submitted patch, 92: 2760167-92.patch, failed testing. View results

markcarver’s picture

Also moved some more duplication into the base class.

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

dawehner’s picture

Above discussion makes me wonder if we should profile this?

Mh, I don't think this is useful. Messages are almost exclusively sent on POST request where we don't even have render caching enabled.

  1. +++ b/core/lib/Drupal.php
    @@ -757,4 +765,17 @@ public static function time() {
    +  public static function messenger() {
    +    if (static::$messenger === NULL) {
    +      static::$messenger = new ChainedMessenger();
    +    }
    +    return static::$messenger;
    +  }
    

    Could we still do some kind of warning, because ideally this case really should not happen, right?

  2. +++ b/core/lib/Drupal/Core/Messenger/BaseMessenger.php
    @@ -0,0 +1,92 @@
    +    return $repeat || !in_array($message, $this->messagesByType($type));
    

    Do you mind using a strict in_array here?

  3. +++ b/core/lib/Drupal/Core/Messenger/ChainedMessenger.php
    @@ -0,0 +1,134 @@
    +/**
    + * Provides a chained Messenger.
    + *
    + * This implementation uses a memory messenger for handling messages before the
    + * container is initialized. When initialised, it will copy over messages and
    + * use that thereafter.
    + *
    + * You should not use this class directly. Instead use the 'messenger' service.
    + *
    + * @internal
    + */
    +class ChainedMessenger implements MessengerInterface {
    

    I think we should have a test which ensures in a unit test that copying the messages over actually happens.

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
26.28 KB
6.05 KB

Re: #94 reverted those changes.

Re: #95

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

Status: Needs review » Needs work

The last submitted patch, 96: 2760167-96.patch, failed testing. View results

kim.pepper’s picture

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

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

markcarver’s picture

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

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

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

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

markcarver’s picture

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

kim.pepper’s picture

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

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

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

markcarver’s picture

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

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

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

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

dawehner’s picture

At least for me #2924538: [META] Remove all usages of drupal_get_message and drupal_set_message might take a long while, so it might be better to have the deprecation being there, so contrib modules can adapt already. On top of that it is a bit weird that we should do something different here than on all the other deprecation issues.

larowlan’s picture

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

jibran’s picture

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

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

larowlan’s picture

Assigned: Unassigned » xjm

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

markcarver’s picture

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

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

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
26.24 KB
773 bytes

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

Status: Needs review » Needs work

The last submitted patch, 108: 2760167-108.patch, failed testing. View results

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
26.28 KB

Re-roll of #108

markcarver’s picture

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

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

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

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

xjm’s picture

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

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

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

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

xjm’s picture

Assigned: xjm » Unassigned

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

dawehner’s picture

I debugged why the test originally failed:

The tests installed the "experimental_module_test" module, which triggers the confirm form.
\Drupal\system\Form\ModulesListExperimentalConfirmForm is then submitted.
As part of building the form before the submission on the POST request \Drupal\system\Form\ModulesListExperimentalConfirmForm::buildMessageList is called (which calls out to drupal_set_message()).
After that the actual form is executed (\Drupal\system\Form\ModulesListConfirmForm::submitForm) which rebuilds the container.

When we use \Drupal::messages(), we effectively has a static, which survives container rebuilds, so the message from \Drupal\system\Form\ModulesListExperimentalConfirmForm::buildMessageList survives the module install and container rebuild.

Note: you can totally reproduce the second failure by simply installing the experimental test module with the patch applied.

I think overall having a static in \Drupal smells really dangerous. What we should rather do is to solve the problem limited to our usecase. Ideally we would rather have something which is not a real static. I though about doing something like:

      // Once the merging is done we ensure that the old messengers is gone, so
      // it does not leak into another instance, like after a container rebuild.
      $this->memoryMessenger = new MemoryMessenger();

but this did not solved it.

markcarver’s picture

Assigned: Unassigned » markcarver
Status: Needs review » Needs work

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

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

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

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

kim.pepper’s picture

Thanks Mark!

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

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

markcarver’s picture

Assigned: Unassigned » markcarver
Status: Needs review » Needs work

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

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

edit: after test results come back

markcarver’s picture

Assigned: markcarver » Unassigned
Status: Needs work » Needs review
FileSize
29.2 KB
5.61 KB

Yay #116 passed!

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

kim.pepper’s picture

Looks great! RTBC if green.

markcarver’s picture

Status: Needs review » Needs work

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

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

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

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

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

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

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

Assigned: Unassigned » markcarver

I meant to assign this to myself.

markcarver’s picture

Title: Add \Drupal\Core\Messenger\Messenger and remove \Drupal\Core\Messenger\LegacyMessenger » Add \Drupal\Core\Messenger\Messenger

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

markcarver’s picture

Assigned: markcarver » Unassigned
Status: Needs work » Needs review
FileSize
29.4 KB
17.3 KB

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

dawehner’s picture

Well, now I could imagine that we no longer need the static on the Drupal class to make it working?

markcarver’s picture

Yep

kim.pepper’s picture

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

markcarver’s picture

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

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

jibran’s picture

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

    Isn't the order of messages important?

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

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

jibran’s picture

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

kim.pepper’s picture

Re #127 Ah ok. Makes sense then.

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

markcarver’s picture

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

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

Status: Needs review » Needs work

The last submitted patch, 126: 2760167-126.patch, failed testing. View results

markcarver’s picture

Status: Needs work » Needs review
FileSize
29.6 KB
2.32 KB

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

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

markcarver’s picture

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

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

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

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

dawehner’s picture

The alternative way to fix this is.

I still try to understand why we need to support pre container messages. I get that we don't want to fatal, no question at all, but why can't we follow the strategy of HEAD aka. ignore messages? Dealing with messages, which are inherently session related, feels utterly wrong, as long we don't have a real session initially yet through our session system (see \Drupal\Core\StackMiddleware\Session).
#2760167-75: Add \Drupal\Core\Messenger\Messenger introduced that, so I'm wondering whether this direction was maybe a bit to drastic.

Status: Needs review » Needs work

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

markcarver’s picture

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

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

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

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

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

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

---

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

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

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

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

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

markcarver’s picture

Status: Needs work » Needs review
FileSize
26.16 KB
8.37 KB

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

markcarver’s picture

Assigned: Unassigned » markcarver
Status: Needs review » Needs work
Related issues: +#2928994: Remove \Drupal\Core\Messenger\LegacyMessenger

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

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

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

markcarver’s picture

Assigned: markcarver » Unassigned
Status: Needs work » Needs review
FileSize
27.22 KB
9.64 KB

Ok, see if this makes more sense.

jibran’s picture

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

> 80

Status: Needs review » Needs work

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

markcarver’s picture

> 80

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

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

markcarver’s picture

Status: Needs work » Needs review
markcarver’s picture

Status: Needs review » Needs work

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

jibran’s picture

markcarver’s picture

Not the point I was making...

markcarver’s picture

Status: Needs work » Needs review
FileSize
27.56 KB
3.96 KB

Re: #136

Yes, prematurely creating a $_SESSION variable causes issues. It's why the patch in that comment, #146 and #141 all failed: it couldn't retrieve the proper session instance in other tests.

Previous patches didn't have this issue because it only did this when the service wasn't available. Thus, I've moved this code from the constructor to just below the @trigger_error() in LegacyMessenger::getMessenger() and added more documentation around why. This still allows us to avoid putting a static in \Drupal::messenger().

This patch should now pass.

markcarver’s picture

Status: Needs review » Needs work

This still allows us to avoid putting a static in \Drupal::messenger().

I lied. This isn't true and \Drupal::messenger() will require a static if we want to transfer messages properly... sigh.

markcarver’s picture

Status: Needs work » Needs review
FileSize
27.72 KB
2.04 KB

The last submitted patch, 150: 2760167-150.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 152: 2760167-152.patch, failed testing. View results

markcarver’s picture

Status: Needs work » Needs review

Seems the testing CI had a quite a few drinks tonight...

markcarver’s picture

It would seem that it's not re-triggering, so here's the same patch.

edit: note to self, there's an "Add test / retest" link underneath the patches, even if they're not on the PIFT results page.

The last submitted patch, 150: 2760167-150.patch, failed testing. View results

The last submitted patch, 152: 2760167-152.patch, failed testing. View results

kim.pepper’s picture

+++ b/core/includes/bootstrap.inc
@@ -466,11 +466,17 @@ function watchdog_exception($type, Exception $exception, $message = NULL, $varia
@@ -498,11 +504,16 @@ function drupal_set_message($message = NULL, $type = 'status', $repeat = FALSE)

+++ b/core/tests/Drupal/KernelTests/Core/Messenger/MessengerTest.php
@@ -0,0 +1,69 @@
+class MessengerTest extends KernelTestBase {

This should really be LegacyMessengerTest now all the logic is in there.

markcarver’s picture

FileSize
27.58 KB
1.37 KB

Sure, that makes sense.

markcarver’s picture

FileSize
27.96 KB
834 bytes

Minor c&p doc fix. Adds an explanation of why the static is needed and a @todo indicating that the static should be removed once LegacyMessenger has been removed.

markcarver’s picture

FileSize
27.96 KB
708 bytes

One too many "proper"...

--English in the AM

kim.pepper’s picture

Thanks Mark. The IS needs an update to explain what the proposed solution is now.

markcarver’s picture

Issue summary: View changes

IS updated. Patch is ready for review and RTBC if nothing glaring is found.

kim.pepper’s picture

Status: Needs review » Reviewed & tested by the community

LGTM!

larowlan’s picture

  1. +++ b/core/lib/Drupal/Core/Messenger/LegacyMessenger.php
    @@ -62,151 +81,95 @@ public function addWarning($message, $repeat = FALSE) {
    +      if (isset($this->messages)) {
    ...
    +        $this->messages = [];
    ...
    +    $this->messages = [];
    

    If we're checking for isset, shouldn't we be setting this to NULL, also ensure's we're cleaning up the $_SESSION value too?

  2. +++ b/core/lib/Drupal/Core/Messenger/LegacyMessenger.php
    @@ -62,151 +81,95 @@ public function addWarning($message, $repeat = FALSE) {
    +    $this->messages[$type] = [];
    

    I think we should unset here too, because an empty array inside an array isn't considered empty.

markcarver’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
28.32 KB
2.31 KB

Addresses #166 as well as being a little more explicit on when to use the the previous $_SESSION['messages'] method (to avoid accidently creating a session).

kim.pepper’s picture

Status: Needs review » Reviewed & tested by the community

Addresses #166 so back to RTBC

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 167: 2760167-167.patch, failed testing. View results

kim.pepper’s picture

Status: Needs work » Reviewed & tested by the community

Re-ran the tests and they passed. Some weirdness with CI bots I think?

Back to RTBC

larowlan’s picture

Status: Reviewed & tested by the community » Needs review

This is so close, one nit and one api surface issue

  1. +++ b/core/lib/Drupal/Core/Messenger/LegacyMessenger.php
    @@ -62,151 +81,104 @@ public function addWarning($message, $repeat = FALSE) {
    +  public function getMessenger() {
    

    Are we sure we want to make this public?

    Looking at the code the only place its called is from tests.

    On that basis I think we should make this protected, and use reflection in the test to make it accessible.

    A smaller API = easier to maintain

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

    nit, two .

kim.pepper’s picture

markcarver’s picture

Status: Needs review » Needs work

The last submitted patch, 173: 2760167-171.patch, failed testing. View results

markcarver’s picture

Status: Needs work » Needs review
FileSize
28.92 KB
612 bytes

Whoops, forgot to change the @covers method too.

kim.pepper’s picture

Status: Needs review » Reviewed & tested by the community

Addresses #171 to back to RTBC

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 175: 2760167-175.patch, failed testing. View results

markcarver’s picture

Status: Needs work » Reviewed & tested by the community

Sigh, #175 passed, random test bot change again.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 175: 2760167-175.patch, failed testing. View results

markcarver’s picture

Status: Needs work » Reviewed & tested by the community

This is getting ridiculous...

larowlan’s picture

Adding review credits

  • larowlan committed 40856cc on 8.5.x
    Issue #2760167 by kim.pepper, markcarver, jibran, dawehner, znerol, Wim...
larowlan’s picture

Status: Reviewed & tested by the community » Fixed

Committed as 40856cc and pushed to 8.5.x.

Published the changed record.

Unpostponed followups.

Thanks again for all the sustained effort here, glad to see we're still moving with these modernisation initiative.

alphawebgroup’s picture

could someone explain, please: why we have named "pulling out messages" methods as "deleting messages"?
I believe, it will mislead developers, especially new developers who is just started working with Drupal
usually "deleting" means "deleting". it usually returns either $this or void.
but our "deleting" does "pulling out", not a "deleting" in common sense.

dawehner’s picture

Title: Add \Drupal\Core\Messenger\Messenger » CCan Add \Drupal\Core\Messenger\Messenger

Can someone please open up a follow up to get rid of this static? I am kind of a strong believer that this static will hurt us rather sooner than later.

markcarver’s picture

Title: CCan Add \Drupal\Core\Messenger\Messenger » Add \Drupal\Core\Messenger\Messenger

The static for LegacyMessenger is needed. I'm curious how this would "hurt us" given that this is only for BC purposes and is, ultimately just proxied to the Messenger service.

  1. +++ b/core/lib/Drupal/Core/Messenger/LegacyMessenger.php
    @@ -62,151 +81,104 @@ public function addWarning($message, $repeat = FALSE) {
    +      // Otherwise, just set an empty array.
    +      else {
    +        $this->messages = [];
    +      }
    

    If there is no $_SESSION, it uses an empty array. This behaves just like the previous MemoryMessenger and requires that \Drupal::messenger() persists. Simply calling new LegacyMessenger() each time would blow away any previously set messages (and would now fail tests that were added here).

  2. +++ b/core/lib/Drupal/Core/Messenger/LegacyMessenger.php
    @@ -62,151 +81,104 @@ public function addWarning($message, $repeat = FALSE) {
    +    if (\Drupal::hasService('messenger')) {
    +      // Note: because the container has the potential to be rebuilt during
    +      // requests, this service cannot be directly stored on this class.
    +      $messenger = \Drupal::service('messenger');
    

    There is no risk with statically storing LegacyMessenger because the messenger service itself is always retrieved and never stored statically. Otherwise, it would never pass tests (as was proven above when you dug into why they were failing on the form submits).

markcarver’s picture

I created the follow-up though, so we can investigate this further if you really feel so strongly about it.

almaudoh’s picture

Great work going on this issue. It's nice to have drupal_{set|get}_message() "OO'ified". Even though this is coming late in the day, but I've observed something:

+  /**
+   * {@inheritdoc}
+   */
+  public function addError($message, $repeat = FALSE) {
+    return $this->addMessage($message, static::TYPE_ERROR);
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public function addMessage($message, $type = self::TYPE_STATUS, $repeat = FALSE) {
+    if (!($message instanceof Markup) && $message instanceof MarkupInterface) {
+      $message = Markup::create((string) $message);
+    }
+
+    // Do not use strict type checking so that equivalent string and
+    // MarkupInterface objects are detected.
+    if ($repeat || !in_array($message, $this->flashBag->peek($type))) {
+      $this->flashBag->add($type, $message);
+    }
+
+    // Mark this page as being uncacheable.
+    $this->killSwitch->trigger();
+
+    return $this;
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public function addStatus($message, $repeat = FALSE) {
+    return $this->addMessage($message, static::TYPE_STATUS);
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public function addWarning($message, $repeat = FALSE) {
+    return $this->addMessage($message, static::TYPE_WARNING);
+  }
+  public function addError($message, $repeat = FALSE) {
+    return $this->addMessage($message, static::TYPE_ERROR);
   }
 
   /**
    * {@inheritdoc}
    */
   public function addMessage($message, $type = self::TYPE_STATUS, $repeat = FALSE) {
-    $this->setMessage($message, $type, $repeat);
+    // Proxy to the Messenger service, if it exists.
+    if ($messenger = $this->getMessengerService()) {
+      return $messenger->addMessage($message, $type, $repeat);
+    }
+
+    if (!isset($this->messages[$type])) {
+      $this->messages[$type] = [];
+    }
+
+    if (!($message instanceof Markup) && $message instanceof MarkupInterface) {
+      $message = Markup::create((string) $message);
+    }
+
+    // Do not use strict type checking so that equivalent string and
+    // MarkupInterface objects are detected.
+    if ($repeat || !in_array($message, $this->messages[$type])) {
+      $this->messages[$type][] = $message;
+    }
+
+    return $this;
   }
 
   /**
@@ -44,13 +70,6 @@ public function addStatus($message, $repeat = FALSE) {
     return $this->addMessage($message, static::TYPE_STATUS);
   }
 
-  /**
-   * {@inheritdoc}
-   */
-  public function addError($message, $repeat = FALSE) {
-    return $this->addMessage($message, static::TYPE_ERROR);
-  }
-
   /**
    * {@inheritdoc}
    */
@@ -62,151 +81,104 @@ public function addWarning($message, $repeat = FALSE) {
    * {@inheritdoc}
    */

The $repeat flag is not propagated to the ::addMessage() method calls in the simplified versions ::addError(), ::addStatus() and ::addWarning(). This also implies there is no test coverage for adding repeat messages.

larowlan’s picture

@almaudoh - thanks can you open a new follow up to resolve that? nice catch

kim.pepper’s picture

Status: Fixed » Closed (fixed)

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

heddn’s picture

Maybe I missed a follow-up issue, but I don't see an @deprecated and @see to the CR on drupal_set_message/drupal_get_messages. Do we need to open one?

From the CR:

The procedural functions drupal_set_message() and drupal_get_messages() have been deprecated and replaced with a dedicated Messenger service.

heddn’s picture

Ignore that last comment. Needed to switch branches.