Maybe we should document somewhere that the included modules also have an explicit line in the .gitignore to include the deployed files such as

!/public/assets/admin/

Since the included modules is not likely to change I am not sure if it is worth adding a test in the deployment script ModuleAssetPublisher.php checking if there are assests from modules that are directly in modules and that have no override in the .gitignore file for their content. Or we just add some note in docs/simplesamlphp-developer-information.md if more primary modules (those in the repo itself) have assets to make sure the .gitignore file is updated for them if they have public assests.

Maybe we should document somewhere that the included modules also have an explicit line in the .gitignore to include the deployed files such as

!/public/assets/admin/

Since the included modules is not likely to change I am not sure if it is worth adding a test in the deployment script ModuleAssetPublisher.php checking if there are assests from modules that are directly in modules and that have no override in the .gitignore file for their content. Or we just add some note in docs/simplesamlphp-developer-information.md if more primary modules (those in the repo itself) have assets to make sure the .gitignore file is updated for them if they have public assests.

I've updated the docs and also added a small unit tests that will check modules in the repo that have assets if they are noted in .gitignore, this should do it...

composer-unused now reports file=composer.json,line=52,col=0::ext-fileinfo is unused but I'm not sure if it is really ok to remove ext-fileinfo from require.

composer-unused now reports file=composer.json,line=52,col=0::ext-fileinfo is unused but I'm not sure if it is really ok to remove ext-fileinfo from require.

Yes, it's only use was the mime_content_type() call in Module.php and that is now gone, so it's safe to remove this from composer.json

@tvdijen Ok, we can check those in other PRs so we don't put more stuff in this one. Fine by me for additional issues for v3 milestone or project...

@monkeyiq I'll go through every comment from you a bit later.

I went over them and it's fair to say we need to get the documentation right immediately, because well.. We all know what happens if we don't :') Other than that I would merge this.

Some of the comments I had were more "nice to have" and I have tried to add that to them when converting from brain-dump in emacs to this PR.

I am also happy to move more to symfony and think that having it in subsequent PRs is a good way to break it up a little for reasonable sized review chunks.

I think the logging change was mentioned in a few places. Symfony comes with some PSR-3 loggers including monolog support.

https://symfony.com/doc/current/logging.html

If there was a common base class or some form of mix in we might be able to have the existing Logger class forward to the injected Symfony logger and then avoid needing to think about that change when logging. I haven't explored that but it might be a handy way to move to that so that as a dev we don't have to think is it Logger::error() or $logger->error() for this release.

One thought for migration...

If modules all need to update their paths on the release maybe we should tell module authors to update paths to using these helpers prior to 3.0? See the part of "docs/simplesamlphp-install.md"

Does it make sense for a late 2.x series release to rewrite /module/ requests to /module.php/ so that modules might already be using the new URL format prior to 3.x?

Maybe we should try to have

host/simplesaml/module.php/cron/...
host/simplesaml/module/cron/...

Huh, good point... since exchanged metadata will not be updated between entities at the exact time of upgrade to v3, bunch of things will break when they try to upgrade, until they refresh their metadata. We definitely need a transition... How about in v3 I simply do a second pass when loading routes and give those a module.php prefix. So we would have both module and module.php of all routes. I could also introduce a config option that would enable that...

So we would have both module and module.php of all routes.

This is only necessary for the metadata-endpoints and other SAML endpoints where we receive SAML-messages.

So we would have both module and module.php of all routes.

This is only necessary for the metadata-endpoints and other SAML endpoints where we receive SAML-messages.

It is also relevant for oidc module which exposes its metadada and relevant endponts in it, I guss casserver also... any endpoint that other services had to register, any API endpoint that is memorized... this will all stop working for them until they update to new URLs

It is also relevant for oidc module which exposes its metadada and relevant endponts in it, I guss casserver also...

Absolutely! Add the adfs-module to that list too!
I got the impression that you wanted to keep the old module.php alive even for stuff like the admin-module where an endpoint-change shouldn't be an issue. We should fix this for the relevant endpoints only, not some generic solution for every route.

It is also relevant for oidc module which exposes its metadada and relevant endponts in it, I guss casserver also...

Absolutely! Add the adfs-module to that list too! I got the impression that you wanted to keep the old module.php alive even for stuff like the admin-module where an endpoint-change shouldn't be an issue. We should fix this for the relevant endpoints only, not some generic solution for every route.

I'm afraid that we don't know all of the relevant endpoints... Those are not just protocol related endpoints. For example, cron module - in our federation we call its endpoints in crons. WIth metadata aggregator we have automations... The problem is, when upgrading to v3 of SimpleSAMLphp I'll have to upgrade all related external stuff that calls SSP endpoints at the same time. I can't do that in advance because new URLs won't work until I upgrade SSP to v3, and old URLs won't work after I upgrade to v3.

I'm in favor of "transition period", where in v3 we will support all routes - new and legacy (with new ones being published in metadata). And in a next major version, or even minor - so in v3.1 - we remove support for legacy routes. This way people will have a chance to announce the change, publish any new metadata, for which ever protocol, change any external calls to SSP endpoints to cron, metadata, whatever - while old routes work. With this, in theory, there shouldn't be downtime regarding the new URL form for any module, even custom module ones...

It is also relevant for oidc module which exposes its metadada and relevant endponts in it, I guss casserver also...

Absolutely! Add the adfs-module to that list too! I got the impression that you wanted to keep the old module.php alive even for stuff like the admin-module where an endpoint-change shouldn't be an issue. We should fix this for the relevant endpoints only, not some generic solution for every route.

I'm afraid that we don't know all of the relevant endpoints... Those are not just protocol related endpoints. For example, cron module - in our federation we call its endpoints in crons. WIth metadata aggregator we have automations... The problem is, when upgrading to v3 of SimpleSAMLphp I'll have to upgrade all related external stuff that calls SSP endpoints at the same time. I can't do that in advance because new URLs won't work until I upgrade SSP to v3, and old URLs won't work after I upgrade to v3.

I'm in favor of "transition period", where in v3 we will support all routes - new and legacy (with new ones being published in metadata). And in a next major version, or even minor - so in v3.1 - we remove support for legacy routes. This way people will have a chance to announce the change, publish any new metadata, for which ever protocol, change any external calls to SSP endpoints to cron, metadata, whatever - while old routes work. With this, in theory, there shouldn't be downtime regarding the new URL form for any module, even custom module ones...

I suggest the following as a solution in transition period: b17496b

Another broad question without a specific answer...

As the AssertionConsumerService updates seem fairly mechanical should we maybe allow /module.php/ to be in there and update that to /module/ for 3.0 so that the same metadata files will work. This will allow limited migration between 2.9 and 3.0 with the same metadata files and if those files are being synced from other sources will allow older ones to continue to work for a limited time.

I suggest the following solution in v3.0 in tranistion period: b17496b

It might be nice for onKernelException() to have an example that also uses TWIG for the returned page. See the example in docs/simplesamlphp-ecp-idp.md which returns some inline html.

Category: Can easily delay until after merge.

Yes, indeed... I've added twig example in docs/simplesamlphp-errorhandling.md (I think you meant that file): b9cf655

In "upgrade-notes-3.0.md" we might like to expand on

For module URLs, use the URL helpers provided by SimpleSAMLphp

to have one or more examples of the code update for this. Maybe just mentioning to use Module::getModuleURL().

Likewise "For assets, use the published asset URLs." having a URL there to allow folks to avoid having to work it out. Perhaps mention the new Module::getModuleAssetUrl().

Indeed... here is the added note: 66a9cc7

For the error handling...

Somewhere in the docs we might like to add

php bin/console debug:event-dispatcher kernel.exception 

to see if the error handler is registered.

As an aside I plan to but have not run a local test on the error handling stuff. Likely an easy exercise but something I plan to do but have not done as yet.

Ok, added: 595d9a4

Easy to delay to after merge: Maybe throw an error if error.show_function remains in place in the config? The downside is that the same config.php couldn't then be used between 2.9 and 3.0. The upside is that the user will not be left thinking they have a custom error handler in 3.0 and not have one because they forgot to migrate that part of their setup. A soft option here is we could have a pedantic_config_check type of option that is recommended but can be set to false to allow a more migration friendly config with the caveat that some options might be ignored and not reported if you force pedantic to false.

Huh... I see what you mean, this could even be implemented in a way to track many config options, and present any deprecated ones in the UI where users will clearly get the message... but I'm now sure about it... but I would certainly like to keep it out of scope of this PR...

I'm in favor of "transition period", where in v3 we will support all routes

I can live with that :) One question though: what would be a reasonable time for the transition period? What is a "normal" interval to refresh metadata? I do this once every three year, because that's the lifetime of our signing keys, however SAML2INT for instance preaches to use long-lived self-signed certificates.. Could easily be 10 yrs?

Other than that, I think this PR is ready to merge.

I notice the path change in docs/simplesamlphp-sp-api.md which I assume is ok but thought I'd mention it to double check...

a link containing

   module.php/core

going to

   module/saml

The related method is below, it points to 'saml/sp/login/...', so the mistake was before..

    public function getLoginURL(?string $returnTo = null): string
    {
        if ($returnTo === null) {
            $httpUtils = new Utils\HTTP();
            $returnTo = $httpUtils->getSelfURL();
        }

        $login = Module::getModuleURL('saml/sp/login/' . urlencode($this->authSource), [
            'ReturnTo' => $returnTo,
        ]);

        return $login;
    }

We should check the docs for any mention of .php if that didn't already happen as part of this PR

I'm in favor of "transition period", where in v3 we will support all routes

I can live with that :) One question though: what would be a reasonable time for the transition period? What is a "normal" interval to refresh metadata? I do this once every three year, because that's the lifetime of our signing keys, however SAML2INT for instance preaches to use long-lived self-signed certificates.. Could easily be 10 yrs?

Other than that, I think this PR is ready to merge.

I don't know what is reasonable timeframe would be.... I didn't introduce a config option to enable or disable legacy URLs in the end, to not pollute config file with something that will be removed anyways in the future. So everyone will have to wait for a new SSP version which will have legacy URLs removed. For which version should this be, you guys can decide this at any time - just remove the one-line in Kernel to stop supporting legacy URLs in some future version release...

We should check the docs for any mention of .php if that didn't already happen as part of this PR

This should already be fixed.

Other than those thoughts I need to pull this and test it locally and I guess then the "should be carefully tested before merge" part you mention in the initial comment phase begins :)

I think I went through all your suggestions. I did try things out while working on this, but it would be great if you guys do that also...

I knew this would have some interaction with #2645

I can take a look at updating the 4 breaks in 2645