[Regression] Flag/unflag messages don't show for reload and confirm link types

Just 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

if ( ajax ) 
 Handle Ajax request
else 
  Ensure appropriate message is send   <- new step
  Generate redirect

return response.

As my eyes wandered over the code I fixed some coding standard errors ... which admittedly are slightly out of scope ... but only small stuff

Minor 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!

Minor 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.

This 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.

Interesting, I can see advantages here.

1) Fixed minor coding standard thing in ActionLinkController

2) Appended a new noJS test method to AjaxLinkTest

Sorry 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

multiple protected methods

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.

I 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

Ah 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...

Finally got the time to review it. ^_^

Thanks all!

Late 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.