Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
I create a flag with the reload link type, set the flag and unflag messages. I don't see these when I flag or unflag.
Split off from #2732065: Flag links redirect to the system path rather than the actual path the user was on.
Comment | File | Size | Author |
---|---|---|---|
#10 | 2834455-10.patch | 16.73 KB | martin107 |
| |||
#10 | interdiff-2834455-9-10.txt | 5.62 KB | martin107 |
Comments
Comment #2
joachim CreditAttribution: joachim commentedChanging name to clarify this issue versus the AJAX link one.
Comment #3
martin107 CreditAttribution: martin107 as a volunteer commentedJust cherry picking a release blocker ..
The solution is to modify ActionLinkController::generateResponse() to add a new step, a call to messenger->addMessage() that is added only when dealing with old school links.
NB drupal_set_message is now deprecated.
Overview of code flow
As my eyes wandered over the code I fixed some coding standard errors ... which admittedly are slightly out of scope ... but only small stuff
Comment #4
socketwench CreditAttribution: socketwench as a volunteer commentedMinor quibble: That method is starting to get pretty big. I'm starting to feel like we should break it up into multiple protected methods.
I'm guessing this needs tests too, but otherwise it looks good!
Comment #5
martin107 CreditAttribution: martin107 as a volunteer commentedMinor quibble
To my way of thinking it is certainly ugly .. has too many if statements ( so the cylomatic complexity score will be poor ) and as a code smell we are passing the request object through too many levels of function calls.
My time for open source stuff, at the moment, is low... I will get to it when I can.
Comment #6
martin107 CreditAttribution: martin107 as a volunteer commentedThis still needs test, but I am posting what I have ..cos the change is big... I am dealing with the minor quibble.
There is no point to the interdiff... as I started from scratch...
I have cleaved the ActionLinkController into two controllers,
ActionLinkController taking control of the response to the conventional AJAX "POST" request handling.
ActionLinkNoJsController taking control of the legacy, "GET" - "NoJS" response handling.
I am not making a performance claim... just trying to justify why I made that conceptual split.
For most sites the ActionLinkNoJsController should not appear in the opcode cache .. unless we have special nojs visitors like bots/web crawlers.
All those annoying context sensing if statements are relegated to Symfony and its routing function and the request object is no longer passed from pillar to post.
More tomorrow.
Comment #7
martin107 CreditAttribution: martin107 as a volunteer commentedComment #8
socketwench CreditAttribution: socketwench at TEN7 commentedInteresting, I can see advantages here.
Comment #9
martin107 CreditAttribution: martin107 as a volunteer commented1) Fixed minor coding standard thing in ActionLinkController
2) Appended a new noJS test method to AjaxLinkTest
Comment #10
martin107 CreditAttribution: martin107 as a volunteer commentedSorry the new test in the last patch ... was a demonstration of posting too early, too often :(
This is better..
1) I've converted the new test into a functional test ( it was a functionalJavascript test )
2) I've make the generateResponse() methods private...
@socketwench from #4 when you say
I guess I take the ( non-drupal ) symfony line about the visibility of a function ( public/protected/private )
That the controller helper subfunction generateResponse() should be private. Protected only if it was likely to overridden which I don't think it is.
I hope that is not too controversial.
3) Minor coding standard fixups.
I think the patch is a complete fix.... now.
Comment #11
socketwench CreditAttribution: socketwench at TEN7 commentedI haven't forgotten about this patch! I really need to remember to clear an hour to read it through carefully though; you're so prolific, Martin! :-D
Comment #12
martin107 CreditAttribution: martin107 as a volunteer commentedAh no problem, I am giggling to myself 'cos I am working on a core patch where the review that toggled the issue to "Needs work" was filed September 2017 and the draft change record was created in 2015...
So I appreciate these things sometimes go slow.. please take your time...
Comment #13
socketwench CreditAttribution: socketwench as a volunteer commentedFinally got the time to review it. ^_^
Comment #15
socketwench CreditAttribution: socketwench as a volunteer commentedThanks all!
Comment #17
AaronBaumanLate to the game here, but I have an implementation that is/was relying on the protected
generateResponse
method.I'm overriding
ActionLinkController
behavior, because I want to add my own custom behavior to the flag response.Updating to a version with this change broke my impelmentation.
Now i have to re-implement the entire method rather than calling
parent::
, which is less than ideal.