-
Notifications
You must be signed in to change notification settings - Fork 242
Upgrade PHP minimum version to 8.2 #644
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 PHP minimum version to 8.2 #644
Conversation
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 suggest reducing the range of PHPUnit versions in the dev requirements, as old versions are not needed anymore for our CI.
composer-flags: [ "" ] | ||
experimental: [ false ] | ||
include: | ||
- php: 7.4 | ||
- php: 8.2 | ||
composer-flags: "--prefer-lowest" | ||
- php: "8.4" # TODO move that to a normal job once phpspec supports PHP 8.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 job should be removed as you solved the TODO.
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 don't think so, it's connected to phpspec minimum version.
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 job is not about a min version at all. It is related to the fact that it was impossible to test 8.4 as a normal job not having the special composer flag to ignore the upper bound of the PHP requirement (because phpspec was not yet declared compatible with PHP 8.4).
The fact that you were able to add 8.4 as a normal job a few lines above is exactly the proof that this todo is resolved (well, partially: you copied it as a normal job instead of moving it, which is why I ask for this removal)
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.
OK, I see that you reverted that part.
1181236
to
37dfbef
Compare
I updated the branch with the changes regarding phpunit versions. Can you have a look to the phpstan error ? I don't understand how to fix it. |
An approval for the Ci would be super-nice @stof :) |
.github/workflows/build.yml
Outdated
@@ -59,7 +59,7 @@ jobs: | |||
- name: Set up PHP | |||
uses: shivammathur/setup-php@v2 | |||
with: | |||
php-version: "8.2" | |||
php-version: "8.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.
use PHP 8.3 here, as dependencies cannot be installed on 8.4 without special flags because of phpspec.
src/Prophecy/Doubler/LazyDouble.php
Outdated
* @template U of object | ||
* @phpstan-param class-string<U>|ReflectionClass<U> $class | ||
* @phpstan-this-out static<U> | ||
* @phpstan-param class-string<T>|ReflectionClass<T> $class |
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.
Please don't remove this. This method is indeed changing the generic so the previous annotation was the right one (being able to call setParentClass
only with the existing class name does not make sense, which is what your new annotation implies)
* @template U of object | ||
* @phpstan-param class-string<U> $class | ||
* @phpstan-this-out static<T&U> | ||
* @phpstan-param class-string<T> $class |
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 must be reverted. willExtend
is about changing the associated type. You are breaking the analysis of usages of the public API with this change, requiring willExtend
to accept only the already configured class.
@@ -26,7 +26,7 @@ | |||
/** | |||
* @author Konstantin Kudryashov <[email protected]> | |||
* | |||
* @template-covariant T of object | |||
* @template T of object |
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.
please revert this change for now. I'm quite sure we are still covariant here.
ececf7e
to
9c22648
Compare
@stof phpstan related changes are reverted (I agree it was designed to make phpstan pass more than reflect the reality of things :/) We're back on my previous message: "Can you have a look to the phpstan error ? I don't understand how to fix it." :( |
which error ? |
I'll deal with that when changing our phpstan job to 8.4 in the future (once we can do that in a simple way) |
All good for me, this can be merged then :) . |
I started to work on #637 but supporting old versions of PHP is a lot of work and adds a layer we do not really need anymore since those versions are not supported anymore by PHP itself.
This is why I'm proposing here a PR that bump the minimum version of PHP.
https://www.php.net/supported-versions
Side note: 8.1 is also removed because a lot changed between 8.1 and 8.2 about types and the end of its support also comes soon enough.
Closes #431