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....
Comment | File | Size | Author |
---|---|---|---|
#33 | 2906147-33-interdiff.txt | 623 bytes | Berdir |
#33 | 2906147-33.patch | 2.25 KB | Berdir |
| |||
#32 | 2906147-32-interdif.txt | 765 bytes | Berdir |
#32 | 2906147-32.patch | 2.24 KB | Berdir |
#31 | 2906147-31.patch | 2.18 KB | martin107 |
|
Comments
Comment #2
martin107 CreditAttribution: martin107 as a volunteer commentedComment #4
martin107 CreditAttribution: martin107 as a volunteer commentedFixed 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
Comment #5
martin107 CreditAttribution: martin107 as a volunteer commentedJust 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
but we should enforce the dependencies in the flag.info.yml with
I have updated the Issue summary with the appropriate warning
Comment #6
martin107 CreditAttribution: martin107 as a volunteer commentedComment #7
martin107 CreditAttribution: martin107 as a volunteer commenteddrupal_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,
Comment #8
martin107 CreditAttribution: martin107 as a volunteer commentedfirst step a little reroll.
Comment #9
kiruba karan CreditAttribution: kiruba karan at Drupal Partners for Innoppl Technologies Pvt. Ltd commentedI 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.
Comment #10
martin107 CreditAttribution: martin107 as a volunteer commentedkiruba 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.
Comment #11
BerdirYou 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.
Comment #12
martin107 CreditAttribution: martin107 as a volunteer commentedThe 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.
Comment #13
socketwench CreditAttribution: socketwench at TEN7 commentedSo 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?
Comment #14
BerdirYes, 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.
Comment #15
martin107 CreditAttribution: martin107 as a volunteer commentedMy 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.
Comment #16
kkalaskar CreditAttribution: kkalaskar at Asentech LLC commentedHello,
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.
I think we really don't need of above code, It showing two messages after uninstalling flag module please find screenshot.
Comment #17
kkalaskar CreditAttribution: kkalaskar at Asentech LLC commentedComment #18
maliknaik CreditAttribution: maliknaik as a volunteer and at Google Summer of Code commentedI've used
MessengerTraits
to get the messenger inside theFlagFormBase.php
andFlagResetForm.php
files and also changed thedrupal_set_message
inflag.install
file inside the functionflag_uninstall
to\Drupal::messenger()
service.Comment #19
TR CreditAttribution: TR commentedMessengerTrait should not be added to FlagFormBase, because FlagFormBase already inherits that from EntityForm. Likewise for FlagResetForm which inherits the trait from ConfirmFormBase.
Comment #20
andriansyah CreditAttribution: andriansyah as a volunteer commentedMessengerTrait removed
Comment #21
andriansyah CreditAttribution: andriansyah as a volunteer commentedComment #22
martin107 CreditAttribution: martin107 as a volunteer commentedThe failing test here ... are a known unrelated issue.
#3038818: AjaxLinkTest is broken
Comment #23
TR CreditAttribution: TR commentedThere 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.
Comment #24
andriansyah CreditAttribution: andriansyah as a volunteer commentedYou are right @TR, my bad,
I forgot to check using Codesniffer about that.
Updating the patch now.
Comment #25
oknateComment #26
martin107 CreditAttribution: martin107 as a volunteer commentedI 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
Comment #27
BerdirThis 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.
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.
Comment #28
martin107 CreditAttribution: martin107 as a volunteer commented27.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.
Comment #29
Berdir27.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.
Comment #30
martin107 CreditAttribution: martin107 as a volunteer commentedThanks for that
I will sort this out this weekend.
Comment #31
martin107 CreditAttribution: martin107 as a volunteer commenteda) I have backed out changes to flag_uninstall() as they are being dealt with by another issue.
b) now using Link::fromTextandUrl()
Comment #32
BerdirLooking at where $url is coming from, we can do even better, resulting in simpler code.
Comment #33
BerdirAh, we need to specify the correct link template now..
Comment #34
TR CreditAttribution: TR commentedThen you don't need the use Link anymore ...
Comment #35
BerdirYeah, removed on commit.
Comment #37
TR CreditAttribution: TR commentedQuestion - 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 ...