@cicnavi @monkeyiq First look?
I think it's already a huge improvement and we could for example fix #2643 with this.

Idea:

I have left out the migration to PSR-17 to interact with the saml2-lib (see old PR)..
We can easily re-introduce this by backporting the saml2-bindings from saml2v6 into samlv4

Edit: Never mind, I see now that Template here is XHTML/Template and so is a response.

Looking at this, modules/admin/src/Controller/Config.php still falls through to the code that does

        return $this->menu->insert($t);

Which by looking at Menu should be then trying to return a Template object from Config diagnostics and main.

Looking at #1760 it doesnt seem there was a Menu.php update there to be picked still.

I will go back to reading and using emacs to make cross check comments before posting to avoid noise

I have read over most of this now. I have not checked it out and run it yet.

NB: This is more of a comment for a future update if we like the idea.

One thought I had was to do with getting query parameters...

$consumerURL = null;
if ($request->query->has('ConsumerURL')) {
    $consumerURL = $request->query->get('ConsumerURL');
}

Looking at the symfony source code it seems that this could be a single line. Though the temptation there is also to use the FILTER to validate things at that time as well. (https://github.com/symfony/symfony/blob/e9a41cc7483070563d0e8264842e064716319e11/src/Symfony/Component/HttpFoundation/InputBag.php#L143)

$consumerURL = $request->query->filter( 'ConsumerURL', null );

Looking at the symfony source code it seems that this could be a single line.

I'm trying to not change anything in this already huge PR that is not related to passing requests/responses back and forth. Also, I would never use ->filter(.., null) because it gives a false sense that we're filtering something when we don't.. Reducing the if/has/get to a oneliner makes sense here.

This PR wouldn't be complete if I left out the saml2-endpoints, but they need the Binding-classes from the saml2v6-lib to properly work with Symfony responses.. Therefore I converted them and then convert the response captured by the v6 bindings back to a v4 message-object. It's not pretty, but we need small steps and this PR is already huge.

Where possible I used the Constants and the exceptions from the new lib.

It looks like IndexTest is the only code in the test suite using simplesamlphp-test-framework/src/BuiltInServer.php

There may be a symfony way to do that instead.

There may be a symfony way to do that instead.

Probably, yes.. Although I think what the test is doing can also be tested in the frontpage controller tests.. It's only testing redirects to specific destinations.

Something strange is going on.. ServiceProviderTest::testACSExpectedIssuerMismatchHardFails is passing locally, but failing on GH Actions

The issue with tests/www/IndexTest.php seemed to happen when I merged the ssp3 branch into this here 42a7252 to try to resolve those 4 conflicts that were created.

The IndexTest test runs ok on the ssp3 timeline commits https://github.com/simplesamlphp/simplesamlphp/commits/simplesamlphp-3.0/ ... cfe871c through 4f5d845

I will dig more into IndexTest tomorrow to try to resolve it. (unless it has been discovered in the meantime).

This also completes ok here...

$ ./vendor/bin/phpunit --display-warnings --no-coverage ./tests/modules/saml/src/Controller/ServiceProviderTest.php

I will be trying to get IndexTest happening again as it is currently rather than chasing other changes for the simple error/redirection testing.

Oh crap, that wasn't supposed to happen :/