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

[Core] Raise minimum PHP requirement to 7.3 #5723

Merged
merged 5 commits into from
Jan 8, 2020
Merged

[Core] Raise minimum PHP requirement to 7.3 #5723

merged 5 commits into from
Jan 8, 2020

Conversation

johnsaigle
Copy link
Contributor

Brief summary of changes

PHP 7.2 is no longer receiving active support as of Nov. 30th, 2019. PHP 7.4 is being released at about the same time.

Typically we only support the last two releases of PHP. This PR brings the minimum requirement up to 7.3 from 7.2.

Detailed changes between these versions can be found here: https://www.php.net/manual/en/migration73.php

I'm adding the Security label because this upgrade will enable all LORIS instances to be fully protected from CSRF attacks via the SameSite cookie.

Links to related tickets (GitHub, Redmine, ...)

@johnsaigle johnsaigle added Release: Add to release notes PR whose changes should be highlighted in the release notes Category: Documentation PR or issue that aims to improve the documentation (test plans, wiki, comments...) Release: Document at release PR whose changes need to be documented in the wiki (or other documentation) at release Category: Security PR or issue that aims to improve security labels Nov 21, 2019
@PapillonMcGill
Copy link
Contributor

Will also require to update README.CentOS7.md files (currently in 22.0-release).

@johnsaigle johnsaigle added State: Needs documentation PR that needs a more exhaustive documentation to proceed Blocking PR should be prioritized because it is blocking the progress of another task labels Nov 25, 2019
@johnsaigle
Copy link
Contributor Author

PHP 7.3 is required to use phpstan, so this is blocking #5356.

@johnsaigle johnsaigle marked this pull request as ready for review November 28, 2019 19:09
@johnsaigle
Copy link
Contributor Author

PHP 7.4 is out so this PR can go ahead.

It's possible that Travis doesn't yet support this in the build environments though.

https://www.php.net/releases/7_4_0.php

@johnsaigle johnsaigle added State: Needs rebase PR that needs to be rebased to proceed (conflicts, wrong branch...) State: Blocked PR or issue awaiting an external event such as the merge or another PR to proceed and removed State: Needs rebase PR that needs to be rebased to proceed (conflicts, wrong branch...) State: Needs documentation PR that needs a more exhaustive documentation to proceed labels Dec 2, 2019
@johnsaigle
Copy link
Contributor Author

Adding Blocked because TravisCI doesn't yet support testing against the PHP 7.4 release.

@johnsaigle johnsaigle changed the base branch from major to master December 3, 2019 17:36
@johnsaigle johnsaigle removed State: Needs rebase PR that needs to be rebased to proceed (conflicts, wrong branch...) State: Blocked PR or issue awaiting an external event such as the merge or another PR to proceed labels Dec 3, 2019
@johnsaigle johnsaigle added the State: Blocked PR or issue awaiting an external event such as the merge or another PR to proceed label Dec 9, 2019
@johnsaigle
Copy link
Contributor Author

johnsaigle commented Dec 9, 2019

Blocked by issue #5762

@johnsaigle johnsaigle added State: Needs rebase PR that needs to be rebased to proceed (conflicts, wrong branch...) and removed State: Needs rebase PR that needs to be rebased to proceed (conflicts, wrong branch...) labels Dec 18, 2019
@driusan driusan removed the State: Blocked PR or issue awaiting an external event such as the merge or another PR to proceed label Jan 6, 2020
@driusan
Copy link
Collaborator

driusan commented Jan 6, 2020

This is no longer blocked, phan was updated.

@maltheism maltheism added the State: Needs rebase PR that needs to be rebased to proceed (conflicts, wrong branch...) label Jan 6, 2020
@johnsaigle johnsaigle removed the State: Needs rebase PR that needs to be rebased to proceed (conflicts, wrong branch...) label Jan 6, 2020
@johnsaigle johnsaigle requested a review from driusan January 7, 2020 16:22
@johnsaigle
Copy link
Contributor Author

@driusan Please review as this is no longer blocked by phan and is also holding up other PRs.

}

$project = \Project::getProjectFromID($project_id);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks like a change in behaviour? Why is this required for 7.3?

Copy link
Collaborator

Choose a reason for hiding this comment

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

other than that if it's passing Travis, it looks like changes are minimal so it's good enough for me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not required. It was an enhancement but I agree it's not very helpful here. I'll revert this file.

Copy link
Collaborator

@driusan driusan left a comment

Choose a reason for hiding this comment

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

looks good pending travis

@driusan driusan merged commit e7a11c5 into aces:master Jan 8, 2020
@johnsaigle johnsaigle removed the Blocking PR should be prioritized because it is blocking the progress of another task label Jan 8, 2020
@ridz1208 ridz1208 added this to the 23.0.0 milestone Jan 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Documentation PR or issue that aims to improve the documentation (test plans, wiki, comments...) Category: Security PR or issue that aims to improve security Release: Add to release notes PR whose changes should be highlighted in the release notes Release: Document at release PR whose changes need to be documented in the wiki (or other documentation) at release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LORIS should start requiring PHP 7.3 on minor + bugfix (and 7.4 on major when it's out)
5 participants