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
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner created an issue. See original summary.

ioana apetri’s picture

ioana apetri CreditAttribution: ioana apetri at OPTASY commented
Assigned: Unassigned » ioana apetri
ioana apetri’s picture

ioana apetri CreditAttribution: ioana apetri at OPTASY commented
Status: Active » Needs review
FileSize
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
CreditAttribution: voleger at GOLEMS GABB, Drupal Ukraine Community for GOLEMS GABB, Drupal Ukraine Community 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 CreditAttribution: ioana apetri at OPTASY commented
Status: Needs work » Needs review
FileSize
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
CreditAttribution: kim.pepper 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 CreditAttribution: ioana apetri at OPTASY commented
Status: Needs work » Needs review
FileSize
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
CreditAttribution: kim.pepper commented
Status: Needs review » Reviewed & tested by the community

Thanks yo30. LGTM.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan as a volunteer and at PreviousNext 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
CreditAttribution: kim.pepper commented
FileSize
2937945-11.patch3.22 KB
2937945-11-interdiff.txt808 bytes

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

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran at PreviousNext 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
CreditAttribution: kim.pepper 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
CreditAttribution: catch at Third and Grove 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.