Change how the OIDC Login URL is used, mark the Redirect for deprecation

[dbhynds]

Summary of Changes

Changes how the OIDC Login URL is retrieved, and prep the Redirect for deprecation. Now that we're not building HTML, the Redirect object doesn't really serve a purpose except to return the URL. We might as well just have the LtiOidLogin do that.

In the next version of the library:

// instead of doing this:
$redirect = $oidLogin->doOidcLoginRedirect($launchUrl, $request);
return redirect($redirect->getRedirectUrl());

// you should do this instead:
return redirect($oidLogin->getRedirectUrl($launchUrl, $request));

Testing

I tested this out on the platform and tests passed: https://gitlab.com/packback/questions/-/pipelines/1105960419

• I have added automated tests for my changes
• I ran composer test before opening this PR
• I ran composer lint-fix before opening this PR

[dbhynds]

The OIDC redirect is used by our application to authenticate with the LMS during an LTI launch. Once we've authenticated, we get back the LTI launch data.

The old doOidcLoginRedirect() method used to build a Redirect object. Then our platform could figure out how use that. Your options were to 1) just get the URL, 2) redirect using the PHP header() function, 3) create an auto-submitting javascript form, or 4) do a weird hybrid of 2 and 3.

I'm taking the opinion here that this package shouldn't have any business deciding how the redirect should be executed. Both options 2, 3, and 4 seem to go against how most modern web frameworks work in practice. We shouldn't be building auto-submitting javascript forms. Nor should we be setting headers and exiting the application. If you want to use either of those approaches in your application, you should do that within your application code.

That leaves us only with option 1 - returning the URL. In that case it seems pointless to have a Redirect object whose only purpose is to return a url. We might as well just have the LtiOidcLogin return that URL directly. Hence, I'm recommend we deprecate doOidcLoginRedirect() in favor of the new getRedirectUrl().

[JesseBarron]

@dbhynds - I have a few questions but other than that I was able to confirm that LTI launch still works on the platform.

[JesseBarron]

Do we know at what version doOidcLoginRedirect will be removed? if so, could we mention that in the comment?

[JesseBarron]

$launchUrl seems important enough to warrant this validtation. Why was it removed?

[JesseBarron]

This might be overkill but shouldn't a deprication warning be also be shown for getRedirectUrl?