search

OpenConext / Stepup-tiqr

tiqr IdP for step-up authentication
Apache License 2.0
3 stars 2 forks source link

Upgrade to SF4 #81

Closed epinxteren closed 3 years ago

epinxteren commented 5 years ago

Migrate to sf4.

MKodde commented 3 years ago

Came back from the dentist, did a functional review. I created a development environment as described in the docs/development.md file. That process was rather smooth, only the required NPM version was not installed. But I already mentioned that in a code review item above.

Some other findings:

  1. On the https://tiqr.example.com/demo/sp page (should also be accessible via the StepUp VM box). I also had the certificate issue we discussed over the phone last week. 'app/config/key.pem' cannot be found. This should probably be fixed in the parameters.yml.dist file, and or in the Stepup-Deploy repo.
    1. We probably want these certs in the config folder and not in the deprecated app folder.
    2. The default values set in the dist file should be synced accordingly.
  2. There are 44 deprecation warnings when visiting the demo/sp page. They should not pose any issues in this Symfony 4.4 version, but IMHO some effort should be taken to get rid of most of them. The ones that take more effort, or require additional development might have to wait for the next upgrade round. But in this effort you can ditch most of them with minimal effort.
MKodde commented 3 years ago

Feel free to merge this and fix some of the points on other PR's/stories. Maybe refer in this PR to those stories to keep things organized.

mroest commented 3 years ago

Came back from the dentist, did a functional review. I created a development environment as described in the docs/development.md file. That process was rather smooth, only the required NPM version was not installed. But I already mentioned that in a code review item above.

Some other findings:

  1. On the https://tiqr.example.com/demo/sp page (should also be accessible via the StepUp VM box). I also had the certificate issue we discussed over the phone last week. 'app/config/key.pem' cannot be found. This should probably be fixed in the parameters.yml.dist file, and or in the Stepup-Deploy repo.

    1. We probably want these certs in the config folder and not in the deprecated app folder.
    2. The default values set in the dist file should be synced accordingly.
  2. There are 44 deprecation warnings when visiting the demo/sp page. They should not pose any issues in this Symfony 4.4 version, but IMHO some effort should be taken to get rid of most of them. The ones that take more effort, or require additional development might have to wait for the next upgrade round. But in this effort you can ditch most of them with minimal effort.

yes, please open additional stories for this since its unrelated to this PR.

MKodde commented 3 years ago

Creating issues for 1.1 and 1.2 is fine with me, but 2 might be part of the SF4 upgrade. Having an installation without excessive deprecation warnings is part of the upgrade. But I'm fine with addressing them on a successive PR to prevent adding to this big change set.

Are you okay with that proposal?

mroest commented 3 years ago

Creating issues for 1.1 and 1.2 is fine with me, but 2 might be part of the SF4 upgrade. Having an installation without excessive deprecation warnings is part of the upgrade. But I'm fine with addressing them on a successive PR to prevent adding to this big change set.

Are you okay with that proposal?

Agree that it should clutter logs with warnings but, the demo/sp is only used for testing and I've checked the deprecation warnings but they are actually from vendor packages?

mroest commented 3 years ago

As discussed with @MKodde we will create additional stories to resolve deprecation warnings.