-
Notifications
You must be signed in to change notification settings - Fork 344
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 required version of PHP to 8.0 #659
Conversation
@loic425 I forgot to remove 4.4 from the build workflow . I guess that's why these builds were failing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey Rimas,
thanks a lot for your contribution! However, I need you to split it into two PR. The first one, should contain only PHP & branch alias change in composer.json. This will land in master, then we will create a new branch - 1.11. After that, we can merge second PR with drop of Symfony 4.4 support - with 1.11 it can still be installed, so we can only do it on master (from 1.12 :))
.github/workflows/build.yml
Outdated
@@ -24,7 +24,7 @@ jobs: | |||
fail-fast: false | |||
matrix: | |||
php: ["8.0"] | |||
symfony: ["^4.4", "^5.4"] | |||
symfony: ["^5.4"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change should be done after branching v1.11. Please revert
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lchrusciel Hmmmmmm, so you're suggesting to keep same requirements in Standard and Sylius?
I thought Standard is meant to be a fork-and-forget template for new projects, and thus there was no reason to keep same "relaxed" requirements in it as in sylius/sylius (which can be used during upgrades from older versions, and so it makes sense to support the older LTS there). Is there a flaw in that way of thinking?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense. Indeed, on new project, starting from 4.4 is a no way to go. @lchrusciel WDTY?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure, that everyone may start with Sf 5.4 deps and the related rest? For all these years related Sylius-Standard template had the same constraints as the main Sylius package. I'm not insisting on this tho
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vvasiloi Do you have an opinion for that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with Lukasz. Testing Sylius-Standard with Symfony 4 will catch issues the users can have when updating an older project to a newer Sylius, but without updating Symfony.
Sometimes it's difficult to update both at once because of other dependencies, both own and 3rd party.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is a fair point. I'll update the PR.
e3ea45b
to
26ad132
Compare
This will be the first part then. The branch name used for this PR doesn't make much sense now, but let's say it doesn't matter. 😃 One thing I'm still not sure of is whether to keep |
Thank you, Rimas! 🥇 |
This PR was merged into the master branch. Discussion ---------- This is a spin-off from #659 and includes #659. It updates composer.json: 1. All Symfony packages which currently require v4.4 or v5.x will now require `^5.4`. 2. The `extra/symfony/require` element is added with the value of `^5.4`. This will mean that whenever `composer require symfony/*` is issued, it will automatically require `^5.4` when possible, so you won't end up with a mix of packages from 5.4 and 6.x. 3. `symfony/flex` requirement bumped to `^2.1` (contains #661 and deprecates #645). Commits ------- b04de77 Drop support for Symfony 4.4 and lock the desired Symfony version at 5.4
This updates composer.json to match the requirements in Sylius/Sylius master: PHP^8.0
and Symfony^5.4
.Also adds a theextra/symfony/require
element with the value of^5.4
. This will mean that whenevercomposer require symfony/whatev
is issued, it will automatically require^5.4
when possible, so you won't end up with a mix of packages from 5.4 and 6.x.Also bumpssymfony/flex
requirement to^2.0
(fixes #645).This updates composer.json to require PHP ^8.0.
Also changes
dev-master
branch alias to1.11-dev
(fixes #635). Although maybe that should already be changed to1.12-dev
instead?