Destination Parameter fix

This works for me and resolves the nasty Logic Exception. Rather than using return though is there a way we can continue to use destination?

Different approach. It reverts some of the changes done in #2863340: Fix/unify usage of redirects. But I think that is a good thing. We don't want to get caught into an infinite loop inside of Drupal around redirects, and this is the easiest way to do that. No interdiff, because different approach.

The alternative approach is seen in #2986046: Infinite redirect if you do not have access to the page.. The patch there around disabling RedirectResponseSubscriber is another way to address this for SAML. But it seems an overly complicated approach. And until recently, SAML was using the upstream library's redirect. There's nothing magic about a redirect. The only downside I see is that it makes things harder if you want to alter the URL. Which we probably didn't want anyway.

I have applied and tested this patch against current dev branch. Forgive me if RTBC is something reserved for maintainers here, but this works as designed and is a relatively small patch.

This appears to preserve the functionality of saml/login?desination=foo, which I think will be important if require_login module integration is desired.

URLs!!! *shakes fist at sky*

I found and read https://www.lullabot.com/articles/early-rendering-a-lesson-in-debugging-... and the links from there, and I think I have a better handle on the issue around cacheability metadata/early rendering and URLs now. (#2863340: Fix/unify usage of redirects was my second attempt at it... and only half succeeded. It's unfortunate one apparently needs to know so many details, in order to create a simple URL string for an uncached redirect response.)

Also, it's... weird that I don't seem to have tested 'destination' redirects properly before committing #2863340.

So... two intermingled things:

1) The use of 'destination'. I agree we want to keep using 'destination', especially because it is somewhat reserved/special in Drupal. We want to implement the 'standard destination functionality'... with the twist that we have a few redirects in between.

2) The LogicException.

The one line that @johnjw59 points out / fixes is buggy, and this one-line change is the only thing needed to fix the exception.

The fix from @heddn also cancels out the unwanted redirect that happens because of the 'destination' parameter. Thanks for pointing to your alternative approach, that gave me more context.

It feels nicer to me, to keep returning a Response object to the controller, rather than let the PHP-Saml library issue a redirect response. Still, I don't want to introduce the kind of overhead in that alternative approach, just in order to keep doing that. So I almost implemented the @heddn patch...

...except I think we can fix the redirection more easily than that. Redirect happens because a 'destination' parameter is present in the original request. Can't we just remove that, then? See the commit I just pushed.

Further obsessing over code comments was done in https://cgit.drupalcode.org/samlauth/commit/?h=8.x-3.x&id=2c84a1383a90eb... (which is now 8.x-3.0-alpha1).

(Before fixing more stuff and working toward a stable version, I might spend some time suggesting some tiny DX improvements to Core surrounding Url::toString(). Because really...)

Just for the record:

The 'tiny DX improvements' I spent some time on proposing, ended up being:

• Warning people about the perils of Url::toString() in a proposed 37-line addition to the method docblock, as per the interdiff in #2735575-66: Make Url::toString(TRUE) explicit by having a Url::generate() method, as well as having GeneratedUrl::toString
• Making CacheableResponses (most importantly its child classes TrustedRedirectResponse and LocalRedirectResponse) accept a GeneratedUrl in its constructor parameter #3032255: Extend Cacheable/RedirectResponse constructor to accept GeneratedUrl; not coded yet but if noone responds I'll probably just put a patch up for review
• Adding a warning to CacheableResponse about why using it is dangerous re. caching, and to just use a RedirectResponse instead if your responses are never cached. (Also #3032255; also not put up a patch yet.)

So... yeah...
After having read #2638686: Correctly handle cache data instead of throwing an Exception in EarlyRenderingControllerWrapperSubscriber() and #2450993-107: Rendered Cache Metadata created during the main controller request gets lost until 133 (and Wim Leers' answer on https://www.lullabot.com/articles/early-rendering-a-lesson-in-debugging-...) and analyzing some Drupal Core code...

• I think I finally have a real handle on things.
• I'm slightly sad that (through an understandable clash of circumstances) it takes that much reading/analyzing to know what is going on;
• To my surprise, I think that what the SamlController should have done to prevent the LogicException, is... not use a TrustedRedirectResponse at all. But just use a plain Symfony RedirectResponse - and then we don't need to care about toString() vs. toString(TRUE). Because that's what Core also does in several places.

I'm still considering doing that, but I'll hold off at least until there's movement until the Core issues.

(...and now to finally further clean up this module...)