Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade to SF4 #81

Merged
merged 18 commits into from
Nov 9, 2020
Merged

Upgrade to SF4 #81

merged 18 commits into from
Nov 9, 2020

Conversation

epinxteren
Copy link
Contributor

@epinxteren epinxteren commented Sep 24, 2019

Migrate to sf4.

@mroest mroest changed the title WIP :Feature/sf4 Upgrade to SF4 Nov 5, 2020
Copy link
Member

@phavekes phavekes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested in our dev-enviroment, and it's still functioning as expected. I'll leave the code-nitpicking to @MKodde

Copy link
Member

@MKodde MKodde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! And thanks for committing atomic changes. That made reviewing a breeze. I'm working on a functional review, but setting up the dev-env is somewhat of a challenge.

@@ -51,7 +51,7 @@ before_script:

# Install dependencies
- cp .env.ci .env
- cp config/packages/parameters.yaml.dist config/packages/parameters.yaml
- cp config/packages/parameters.yml.dist config/packages/parameters.yml
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick, consider squashing this with bd0848aa^^

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be honest. IMO we should squash this whole PR since the individuals commits are worthless.

.env.dist Outdated Show resolved Hide resolved
composer.json Outdated Show resolved Hide resolved
config/packages/prod/monolog.yaml Outdated Show resolved Hide resolved
Copy link
Member

@MKodde MKodde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my functional test, found two more findings. They both are touching on the Node/NPM version that is being installed.

homestead/after.sh Outdated Show resolved Hide resolved
.travis.yml Show resolved Hide resolved
@MKodde
Copy link
Member

MKodde commented Nov 9, 2020

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
Copy link
Member

MKodde commented Nov 9, 2020

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
Copy link
Contributor

mroest commented Nov 9, 2020

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
Copy link
Member

MKodde commented Nov 9, 2020

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
Copy link
Contributor

mroest commented Nov 9, 2020

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
Copy link
Contributor

mroest commented Nov 9, 2020

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

@mroest mroest merged commit 1aa2b68 into develop Nov 9, 2020
@mroest mroest deleted the feature/sf4 branch November 9, 2020 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants