Deprecate the special "returnto" parameter and add support for "destination" instead

Yea this is a known issue, that's why I added support for "returnto" as an alternate way to redirect users after login. But that doesn't work when other systems in Drupal generate links with the destination param in them.

I think we can actually fix this and deprecate the "returnto" parameter. I did a little proof of concept and all we should need to do is remove the "destination" parameter from the request object before our controllers return the redirect to the CAS server. That way core's RedirectResponseSubscriber won't see it and redirect users there instead. Though, not sure why I didn't go with that approach initially. Maybe there's a reason...

Changing this to a feature request

@bkosborne, could you, please, share your PoC, mentioned in #5, as a MR so we can look into?

Moving to 2.x

MR opened. It's really just a one line addition to the force login controller. This would allow users to create links like /cas?destination=/some/path. I removed an unused test in the MR as well as cleaned up some comments.

Adding the line didn't break any tests, but I think it may need some new ones.

Thank you. I'll look into

^ thanks, not sure how I did that!

Assigning to work on it.

Everything seems in place. However, I'm waiting for the cas_mock_server update to be merged.

Upstream changes to cas_mock_server are merged! Thanks for working on this!

I'll give this a review today. Thank you

I see there are coding standards failures. My proposal is fix & enforce coding standards in #3231319: Enforce coding standards.

I re-based this against 2.x after merging #3231319: Enforce coding standards and resolved one minor conflict, though I quickly realized that doing so may not be a best practice, as you now cannot easily do a git pull on your local. You have to fetch and then reset --hard to the origin branch, sorry about that. Maybe you would have preferred I simply merged in 2.x.

Setting to needs work for my comment above about PHP 7.1, but other than that this looks good.

Looks like it was introduced in PHP 7.1, and I think Drupal 8.x still supports down to PHP 7.0.8. Drupal 8 is unsupported in November though, and Drupal 9's minimum is PHP 7.3.

That's a good point. Either we postpone or we add 7.1 in CAS composer.json. With the later, sites still on 7.0 should pin 2.0.0-beta2.

But what if we remove the strict typing of the param instead of making it nullable? I think we don't break BC.

The last commit is a tentative to support PHP 7.0.8

Looks good to me. Good to merge in!

Fixing credits. Thank you all.

I am confused by this

I user to use "?returnto=/some/path" and it worked up until somehwat recently.

This module's documentation is confusing on the matter

Users can log in to your site with CAS a number of ways:

Visit /cas
Visit /caslogin (exact same behavior as above)
Visit /user/login and configure CAS to display a CAS login link on the page (See "General Settings" on the CAS settings form)

After a user logs in via CAS, they are returned to the homepage of your site by default. However, you can override this by setting a special "returnto" parameter in the login URL. For example: https://sample.com/caslogin?returnto=/some/other/page. Drupal's standard method for returning users to some other page via the "destination" query param does not currently work, but may after #3150047: Deprecate the special "returnto" parameter and add support for "destination" instead is addressed.

Was "returnto" the old way and now we should use "destination"? does the value of the param need to be a full uri or just a drupal path? url encoded?

Sorry about that, I've corrected the outdated documentation.

You should be using the "destination" param now, not "returnto". The value should be a Drupal path, and I believe should be encoded as any other query string parameter would be.

thanks!