Problem/Motivation

As part of #2924538: [META] Remove all usages of drupal_set_message and drupal_get_messages it would totally make sense to add a method to both ControllerBase and FormBase given that these are the places which should have this service conceptually.

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#11 2937945-11-interdiff.txt808 byteskim.pepper
#11 2937945-11.patch3.22 KBkim.pepper
#8 base_system-add_messenger-2937945-8-D8.patch2.41 KBioana apetri
#5 base_system-add_messenger-2937945-5-D8.patch1.41 KBioana apetri
#3 base_system-add_messenger-2937945-3-D8.patch1.83 KBioana apetri

Comments

dawehner created an issue. See original summary.

ioana apetri’s picture

ioana apetri commented
Assigned: Unassigned » ioana apetri
ioana apetri’s picture

ioana apetri commented
Status: Active » Needs review
StatusFileSize
new base_system-add_messenger-2937945-3-D8.patch1.83 KB

Here is my patch covered the messenger service into both classes.
Please review it.Thanks:)

voleger’s picture

voleger
he/his
Primary language Ukrainian
Location Ukraine, Rivne
commented
Status: Needs review » Needs work

Why not use MessengerTrait that mentioned in #2924538: [META] Remove all usages of drupal_set_message and drupal_get_messages?
It would be better for DX.

ioana apetri’s picture

ioana apetri commented
Status: Needs work » Needs review
StatusFileSize
new base_system-add_messenger-2937945-5-D8.patch1.41 KB

Here is my changes(using the trait)

Status: Needs review » Needs work

The last submitted patch, 5: base_system-add_messenger-2937945-5-D8.patch, failed testing. View results

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented

You will need to copy the Trait code over as well, as it doesn't exist in head.

ioana apetri’s picture

ioana apetri commented
Status: Needs work » Needs review
StatusFileSize
new base_system-add_messenger-2937945-8-D8.patch2.41 KB

I thought that should not be included. Sorry for the mistake.Heere is the new patch. thanks:)

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Status: Needs review » Reviewed & tested by the community

Thanks yo30. LGTM.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Reviewed & tested by the community » Needs review

Should we be adding a single usage here so we get some implied test coverage?

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
StatusFileSize
new 2937945-11.patch3.22 KB
new 2937945-11-interdiff.txt808 bytes

I converted \Drupal\aggregator\Controller\AggregatorController to use it.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
Status: Needs review » Reviewed & tested by the community

#10 is addressed so back to RTBC.

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Issue tags: +messenger

Tagging

  • catch committed b9edf92 on 8.6.x 
    Issue #2937945 by yo30, kim.pepper, dawehner, voleger, larowlan: Add...

  • catch committed 774ee68 on 8.5.x 
    Issue #2937945 by yo30, kim.pepper, dawehner, voleger, larowlan: Add...
catch’s picture

catch
he/him
Primary language English
commented
Version: 8.6.x-dev » 8.5.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.6.x and cherry-picked to 8.5.x. Thanks!

Status: Fixed » Closed (fixed)

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