Drupal 8.5.x has introduced a new messenger service.

recently drupal_set_message() has been deprecated

#2760167: Add \Drupal\Core\Messenger\Messenger - and continual maintenance patches are better than a single monster patch.

CommentFileSizeAuthor
#35 interdiff-2931217-34-35.txt454 bytesmartin107
#35 2931217-35.patch14.85 KBmartin107
#34 interdiff-2931217-33-34.txt602 bytesmartin107
#34 2931217-34.patch14.57 KBmartin107
#33 2931217-33.patch14.57 KBmartin107
#31 2931217-31.patch18.25 KBmartin107
#28 interdiff-26-28.txt11.64 KBthalles
#28 2931217-28.patch18.25 KBthalles
#26 2931217-26.patch7.6 KBmartin107
#19 interdiff-2931217-17-19.txt4.33 KBmartin107
#19 2931217-19.patch19.39 KBmartin107
#17 interdiff-2931217-15-17.txt1.15 KBmartin107
#17 2931217-17.patch17.2 KBmartin107
#15 2931217-15.patch17.21 KBmartin107
#15 interdiff-2894261-12-15.txt11.01 KBmartin107
#12 2931217-12.patch17.44 KBmartin107
#6 interdiff-2931217-4-6.txt487 bytesmartin107
#6 2931217-6.patch24.8 KBmartin107
#4 2931217-4.patch24.45 KBmartin107
#4 interdiff-2931217-2-4.txt6.08 KBmartin107
#2 message-2931217-2.patch23.02 KBmartin107
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

martin107 created an issue. See original summary.

martin107’s picture

Assigned: martin107 » Unassigned
Status: Active » Needs review
FileSize
23.02 KB

one less hidden dependancy.

Status: Needs review » Needs work

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

martin107’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
6.08 KB
24.45 KB

updated this a little.

Updating the issue summary to highlight that this patch valid on Drupal 8.5.x but that this will break on Drupal 8.4.x

Status: Needs review » Needs work

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

martin107’s picture

Status: Needs work » Needs review
FileSize
24.8 KB
487 bytes

fixing another minor err - while I see it.

Status: Needs review » Needs work

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

moshe weitzman’s picture

I'm not sure its prudent to commit this now, as providing bug fixes for 8.4 will be more complicated (requires backports). Also, this patch should require 8.5 in devel.info.

willzyx’s picture

Status: Needs work » Postponed

@martin107 thanks for the patch! as stated in #8 we need to postpone this issue for now

martin107’s picture

As well as being postponed .. I think the patch needs to be discarded and a alternative approach taken.

When this patch lands ...

#2935255: Remove all usages of drupal_set_message and drupal_get_messages in core lib

all the cases in the patch extend FormBase or ControllerBase so we pick up the MessengerTrait

- drupal_set_message("Sample text");
+ $this->messenger()->addStatus("Sample text");

martin107’s picture

Assigned: Unassigned » martin107

So my intent here is to work on this again after 3 months ....

Get it into a converted working state and then postpone again while we talk about the correct time to commit this....

Today core ( 8.6.x ) has committed conversions of a bunch of these.

Specifically in relation to this issue I see that

#2935256: Remove all usages of drupal_get_message and drupal_set_message in modules

has given ConfigFormBase the MessengerTrait - which resolves the issues I was talking about in #10

martin107’s picture

FileSize
17.44 KB

a) From #8

Also, this patch should require 8.5 in devel.info.

As appropriate, I have modified three info files.

b) As per the change record, the messenger service should probably be directly injected into any custom service rather than using the trait functionality.

c) I have simplified the constructor's of classes that extend base classes.

d) Something new to this issue ... I had to refactor devel_generate/devel_generate.batch.inc

e) See DevelGenerateBase there was a minor maintenance hazard - $type defaulted to 'sting' --- if MessengerInterface ever did anything crazy like updated the value of TYPE_STATUS .. we would be out of sync with that craziness. So I made this change.

protected function setMessage($msg, $type = MessengerInterface::TYPE_STATUS) {

So now we have to discuss Moshe Weitzman's "prudent" comment from #8

PS The last patch need a reroll, I said I was going to start from scratch in any event so no interdiff.

martin107’s picture

Assigned: martin107 » Unassigned
Status: Postponed » Needs review

Go testbot go.

Status: Needs review » Needs work

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

martin107’s picture

Status: Needs work » Needs review
FileSize
11.01 KB
17.21 KB

My bad,

$this->messenger is wrong

$this->messenger() is good

I cut and paste that from the change record... so I am about to update the change record...sorry for the distraction.

Status: Needs review » Needs work

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

martin107’s picture

Status: Needs work » Needs review
FileSize
17.2 KB
1.15 KB

Locally DevelQueryDebugTest now passes.

msankhala’s picture

Status: Needs review » Needs work
+++ b/devel.info.yml
@@ -2,7 +2,7 @@ type: module
+core: 8.5

+++ b/devel_generate/devel_generate.info.yml
@@ -2,6 +2,6 @@ type: module
+core: 8.5

+++ b/webprofiler/webprofiler.info.yml
@@ -2,7 +2,7 @@ name: Web Profiler
+core: 8.5

core key should always specify major version of drpual like 7.x, 8.x not the specific release of core.
https://www.drupal.org/docs/8/theming-drupal-8/defining-a-theme-with-an-...

There are few more places where drupal_set_message is left.

$ grep -rn drupal_set_message .
./devel.module:151:    drupal_set_message($msg, $type, TRUE);
./devel.module:265:        drupal_set_message($msg, ($severity_level <= RfcLogLevel::NOTICE ? 'error' : 'warning'), TRUE);
./devel.module:339: * Uses drupal_set_message().
./devel.module:346: *   Optional message type for drupal_set_message(), defaults to 'status'.
./devel.module:363: * Uses drupal_set_message().
./kint/kint.module:56:    drupal_set_message(Markup::create($msg));
martin107’s picture

Status: Needs work » Needs review
FileSize
19.39 KB
4.33 KB

Thanks for the quick review... I hope I have corrected everything.

Prashant.c’s picture

@martin107

Great work.

I tested it for couple of scenarios and it worked fine.

I have a question should we consider MessengerTrait for the same purpose ?

martin107’s picture

I have a question should we consider MessengerTrait for the same purpose ?

A decision tree for when to work with traits

1) If it is a service you are creating then avoid the trait and directly inject into the constructor

2) If it is a small controller method, and it is appropriate to extend ControllerBase, then look for the trait in the base class

3) If what you are writing can be considered application code, then use traits

4) Testing can become complicated. so if you need 100% coverage then consider converting a trait into DI logic.

I hope this helps.

Prashant.c’s picture

Thanks @martin107

This clarifies and definitely very helpful.

moshe weitzman’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll
martin107’s picture

Assigned: Unassigned » martin107

With a DrupalCon starting... I should say I am working on this.

martin107’s picture

Version: 8.x-1.x-dev » 8.x-2.x-dev
martin107’s picture

Assigned: martin107 » Unassigned
Status: Needs work » Needs review
FileSize
7.6 KB

I had to undo do dependency injection ( in the constructor ) because head had already converted many classes to use the trait. No biggie.

thalles’s picture

Status: Needs review » Reviewed & tested by the community

Looks good!

thalles’s picture

I took the opportunity to replace the syntax of the matrices in the files involved.

thalles’s picture

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

Status: Needs review » Needs work

Fails to apply due to today's changes. sorry.

martin107’s picture

Status: Needs work » Needs review
FileSize
18.25 KB

First step, reroll.

No conflicts just auto merging.

martin107’s picture

Issue tags: -Needs reroll
martin107’s picture

Note to self -- when there a DrupalCon going I must pull everyday.

martin107’s picture

martin107’s picture

FileSize
14.85 KB
454 bytes

Another fix.

thalles’s picture

Looks good!

willzyx’s picture

Title: drupal_set_message() is deprecated » Replace calls to deprecated function drupal_set_message with calls to the messenger service

Retitling

willzyx’s picture

amarphule’s picture

#35 applied clean and works for me on Drupal-8.6.15 and devel 8.2-2.x

moshe weitzman’s picture

Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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