Please note this patch makes use of code found in D8.5.x!

I want is to get rid of procedural code as much as possible

and core has just replaced drupal_set_message with a messenger service.

Here is a change that has just hit the Drupal 8.5.x branch

https://www.drupal.org/node/2278383

and its associated change record

https://www.drupal.org/node/2774931

I say "where reasonable" because there are call to drupal_set_message() in FlagFormBase and I am still debating with myself as to what is reasonable.

As a abstract Base class the messenger service should be dynamically pull in on first use within a method call getMessenger()
and then cached --

see for a equivalent example - FormBase

  /**
   * Gets the request object.
   *
   * @return \Symfony\Component\HttpFoundation\Request
   *   The request object.
   */
  protected function getRequest() {
    if (!$this->requestStack) {
      $this->requestStack = \Drupal::service('request_stack');
    }
    return $this->requestStack->getCurrentRequest();
  }

Our change to FlagFormBase would look like

$this->getMessenger()->addMessage('Status update');

and I am not sure it is worth it. -- I am easy so will go with what other think....

CommentFileSizeAuthor
#33 2906147-33-interdiff.txt623 bytesBerdir
#33 2906147-33.patch2.25 KBBerdir
#32 2906147-32-interdif.txt765 bytesBerdir
#32 2906147-32.patch2.24 KBBerdir
#31 2906147-31.patch2.18 KBmartin107
#31 interdiff-2906147-25-31.txt1.37 KBmartin107
#25 2906147--interdiff-24-25.txt2.04 KBoknate
#25 flag-drupal-messenger-2906147-25.patch2.52 KBoknate
#24 2906147_24.patch1.95 KBandriansyah
#24 interdiff_20-24.txt781 bytesandriansyah
#20 interdiff_18-20.txt577 bytesandriansyah
#20 2906147_20.patch2.53 KBandriansyah
#18 flag-message-2906147.patch3.02 KBmaliknaik
#16 Screenshot from 2019-03-17 01-38-50.png180.88 KBkkalaskar
#16 2906147-16.patch5.36 KBkkalaskar
#9 Flag patch for current stable core version.png61.71 KBkiruba karan
#9 Flag Reset Message after patch.png80.86 KBkiruba karan
#8 2906147-8.patch2.5 KBmartin107
#5 interdiff-2906147-4-5.txt275 bytesmartin107
#5 2906147-5.patch2.52 KBmartin107
#4 interdiff-2906147-2-4.txt797 bytesmartin107
#4 2906147-4.patch2.25 KBmartin107
#2 2906147-2.patch1.87 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
Priority: Normal » Minor
Status: Active » Needs review
FileSize
1.87 KB

Status: Needs review » Needs work

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

martin107’s picture

Status: Needs work » Postponed
FileSize
2.25 KB
797 bytes

Fixed coding standard nits.

The errors here are not related to changes introduced by the patch

We have a branch blocker -- which I intended to resolve as part of

#2898502: Convert/Modernise FlagContextualLinksTest

see

https://www.drupal.org/node/2898502#comment-12245973

martin107’s picture

Issue summary: View changes
FileSize
2.52 KB
275 bytes

Just adding a small detail while it is fresh in my head....

The messenger service is new in Core 8.5.x and so this patch needs the appropriate adjustments to lockout sites which have not been upgraded to that release.

IF we had a composer.json file .. which is a whole other topic

we would need a require section

   "require": {
      "drupal/core": "~8.5"
   }

but we should enforce the dependencies in the flag.info.yml with

dependancies:
  - drupal:system(>=8.5.0)

I have updated the Issue summary with the appropriate warning

martin107’s picture

Title: Replace drupal_set_,message where reasonable. » Replace drupal_set_message where reasonable.
martin107’s picture

Title: Replace drupal_set_message where reasonable. » Replace drupal_set_message
Assigned: Unassigned » martin107

drupal_set_message is now deprecated

#2760167: Add \Drupal\Core\Messenger\Messenger

Here is the change record.

https://www.drupal.org/node/2774931

So I am going to update the issue to update as much as possible,

martin107’s picture

Status: Postponed » Needs review
FileSize
2.5 KB

first step a little reroll.

kiruba karan’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
80.86 KB
61.71 KB

I can able to apply patch through command prompt. I tested the functionality both core stable and dev version. As we expected, current stable version doesn't have service messenger. So its throwing error while reset. Status message display perfectly for latest Dev version(8.5.0-dev). Here I attached the screenshot of both versions.

martin107’s picture

Assigned: martin107 » Unassigned
Issue summary: View changes
Related issues: +#2931240: Move to 8.x-5.x branch

kiruba karan --- thanks for that observation

yes Drupal 8.5.x .. and all implications that you expose.

( I tried to express that on the first line of the issue summary )

yes maybe time to advance the version number ... I am staring a new issue to make sure the idea get pushed to a wider audience.

Berdir’s picture

You really shouldn't do that. There is absolutely no reason to hurry with this, drupal_set_message() will stick around for a while and 8.5.x will only become stable in march.

martin107’s picture

There is absolutely no reason to hurry with this,

The view from the other side of the see-saw :-

I am justifying my time working here as a way of seeing problem early so I look informed about the latest coming and going with regard to module development - professionally.

So the contradictions here ... we are in beta and will be for the foreseeable future and yet the module is being used heavily in production.

So the common ground between us...

I am happy to act as if this is a large project .. and follow the standard pattern of develop against 8.5.x and I will work on backporting to stable as a priority.

Anyway I hope we can service both goals .. and will follow the will of the committers.

socketwench’s picture

Status: Reviewed & tested by the community » Postponed

So I have to agree with Berdir on this one. There's no reason to hurry with this particular patch. Maybe we can tag this so we can get back to it when Drupal 8.5 is released?

Berdir’s picture

Yes, not sure I understand the argument. There's nothing to backport or cherry-pick, we just need to wait until 8.5.0 is released and then it can be committed.

martin107’s picture

My phrasing does look awkward in the light of day

shoulda coulda woulda

Should we have gone down the other road... then I would have supported/helped out on maintaining parallel branches .. in this instance there would be no backporting .. it is just that the dev branch advances by one commit and then as soon as the branches divergence then those things need to be considered.

kkalaskar’s picture

Hello,

As per the above discussion, I came to know this thread postponed for release of drupal 8.5. Now drupal 8.5 is out and i guess we can address this issue. So created new patch and drupal_set_message replaced by Messenger service.
https://www.drupal.org/node/2774931

Also one more point we need to consider about .install file in flag module. .install file contains hook_uninstall with drupal_set_message.

/**
 * Implements hook_uninstall().
 */
function flag_uninstall() {
  drupal_set_message(t('Flag has been uninstalled.'));
}

I think we really don't need of above code, It showing two messages after uninstalling flag module please find screenshot.

kkalaskar’s picture

Status: Postponed » Needs review
maliknaik’s picture

I've used MessengerTraits to get the messenger inside the FlagFormBase.php and FlagResetForm.php files and also changed the drupal_set_message in flag.install file inside the function flag_uninstall to \Drupal::messenger() service.

TR’s picture

Status: Needs review » Needs work

MessengerTrait should not be added to FlagFormBase, because FlagFormBase already inherits that from EntityForm. Likewise for FlagResetForm which inherits the trait from ConfirmFormBase.

andriansyah’s picture

andriansyah’s picture

Status: Needs work » Needs review
martin107’s picture

The failing test here ... are a known unrelated issue.

#3038818: AjaxLinkTest is broken

TR’s picture

Status: Needs review » Needs work

There are two "+use Drupal\Core\Messenger\MessengerTrait;" statements that still need to be removed from the patch. As @martin107 said, the test fails are not because of the patch so you can ignore those.

andriansyah’s picture

Status: Needs work » Needs review
FileSize
781 bytes
1.95 KB

You are right @TR, my bad,
I forgot to check using Codesniffer about that.
Updating the patch now.

oknate’s picture

martin107’s picture

I have retested and a failed test turn green... making it review-able again.

Why did I think it was a good thing to do... In short Bedir fixed a long running test instability problem here

https://www.drupal.org/project/flag/issues/2983268#comment-13270669

Berdir’s picture

Status: Needs review » Needs work
  1. +++ b/flag.install
    @@ -62,7 +63,7 @@ function flag_schema() {
      */
     function flag_uninstall() {
    -  drupal_set_message(t('Flag has been uninstalled.'));
    +  \Drupal::messenger()->addMessage(new TranslatableMarkup('Flag has been uninstalled.'));
    

    This hooks seems pretty weird honestly. No other module does this, why should flag?

    Maybe just remove it? Slightly off-topic, but I'm fine with that.

  2. +++ b/src/Form/FlagFormBase.php
    @@ -303,13 +303,18 @@ abstract class FlagFormBase extends EntityForm {
         $url = $flag->urlInfo();
    +    $message_params = ['%label' => $flag->label()];
    +    $logger_params = [
    +      '%label' => $flag->label(),
    +      'link' => $this->l($this->t('Edit'), $url),
    +    ];
         if ($status == SAVED_UPDATED) {
    -      drupal_set_message($this->t('Flag %label has been updated.', ['%label' => $flag->label()]));
    -      $this->logger('flag')->notice('Flag %label has been updated.', ['%label' => $flag->label(), 'link' => $this->l($this->t('Edit'), $url)]);
    +      $this->messenger()->addMessage($this->t('Flag %label has been updated.', $message_params));
    +      $this->logger('flag')->notice('Flag %label has been updated.', $logger_params);
         }
         else {
    -      drupal_set_message($this->t('Flag %label has been added.', ['%label' => $flag->label()]));
    -      $this->logger('flag')->notice('Flag %label has been added.', ['%label' => $flag->label(), 'link' => $this->l($this->t('Edit'), $url)]);
    +      $this->messenger()->addMessage($this->t('Flag %label has been added.', $message_params));
    +      $this->logger('flag')->notice('Flag %label has been added.', $logger_params);
         }
    

    Changing this logic to move the params out seems a bit unrelated, but if we're actually touching fix the usage of $this->l(), which is deprecated too.

martin107’s picture

Status: Needs work » Needs review
Related issues: +#3090081: Remove flag_uninstall()

27.1 yes I think it should be simplified ( so I spawn a new issue )

I have tried outlining a complex throught in response to #27.2

https://www.drupal.org/project/flag/issues/3042758#comment-13324883

In short I am committing to rerolling and resolving issues where they arise

So I think we should leave 27.2 in the patch and commit it. and let me deal with the cleanup.

Berdir’s picture

27.2 wasn't really proper english, but to be clear, I'm proposing to do *more*, not less. The patch moves around the deprecated $this->l() and my suggestion is to convert that to Link::fromTextAndUrl() or what it's called again, instead of having to touch it again.

martin107’s picture

Assigned: Unassigned » martin107

Thanks for that

I will sort this out this weekend.

martin107’s picture

a) I have backed out changes to flag_uninstall() as they are being dealt with by another issue.

b) now using Link::fromTextandUrl()

Berdir’s picture

Looking at where $url is coming from, we can do even better, resulting in simpler code.

Berdir’s picture

Ah, we need to specify the correct link template now..

TR’s picture

Then you don't need the use Link anymore ...

Berdir’s picture

Status: Needs review » Fixed

Yeah, removed on commit.

  • Berdir committed 492ec49 on 8.x-4.x
    Issue #2906147 by martin107, Berdir, andriansyah, kkalaskar, oknate,...
TR’s picture

Question - why was I excluded from getting credit for this issue? I don't claim to have done a lot, but I did more than some who *were* given credit ...

Status: Fixed » Closed (fixed)

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