PHP 8.0 and PHP 8.1 support, BetterReflection 5.x upgrade #324
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Ocramius
commented
Nov 6, 2021
src/DetectChanges/BCBreak/FunctionBased/ParameterTypeContravarianceChanged.php
Outdated
Show resolved
Hide resolved
src/DetectChanges/BCBreak/FunctionBased/ReturnTypeCovarianceChanged.php
Outdated
Show resolved
Hide resolved
|
Test failures are caused by Problem is: @azjezz do you think we could fix that in |
Closed
Ocramius
changed the title
|
done https://github.com/azjezz/psl/releases/tag/1.9.1 |
|
Interesting, PHPUnit with lowest dependencies still fails 🤔 EDIT: I'll add a |
|
Funny, lowest dependency runs now run forever:
|
Ocramius
commented
Nov 6, 2021
This patch brings in PHP 8.0 and 8.1 support through the massive work done by @kukulich on `roave/better-reflection` over the past few months. The upgrade is mostly smooth, but we have some rough edges with: * [x] `TypeIsCovariant`, which needs `ReflectionType`s as well as `Reflector`s * [x] `TypeIsContravariant`, which needs `ReflectionType`s as well as `Reflector`s * [ ] `ReturnTypeCovarianceChanged` uses hacky reflection to get a `Reflector` from a `ReflectionFunctionAbstract` (internal state!) * [ ] `ParameterTypeContravarianceChanged` uses hacky reflection to get a `Reflector` from a `ReflectionFunctionAbstract` (internal state!) * [ ] we miss tests for covariance/contravariance with `ReflectionUnionType` and `ReflectionIntersectionType` (`infection/infection` will highlight that)
Sadly, `symfony/symfony` declared `php:>=7.1` compatibility many moons ago: that is generally technical lack of vision and responsibility on their end, but that ship has sailed. This means that a `composer update` can and **will** install a dependency graph that is incompatible with your system's dependencies. This change prevents installation of `symfony/process` in versions too archaic to actually function. As a side-effect, `vimeo/psalm` was also upgraded to its latest and greatest. Ref: https://github.com/vimeo/psalm/releases/tag/4.12.0
Ocramius
force-pushed
the
feature/php-8-compatibility
branch
from
d52e4f5 to
cfc9528
Compare
…Reflection@1f7a13b This also handles the upcoming BC break of Roave/BetterReflection#900 Some big changes: * `PropertyDocumentedTypeChanged` no longer exists: we use only `PropertyTypeChanged` for now. This is a step backwards, but it is needed to prevent issues around docblocks being too advanced for this library to parse them, for now. * see CuyZ/Valinor#6 * see https://github.com/phpstan/phpstan-src/tree/447cd102d946a2d9883ac82793195275f54296a9/src/Type * see https://github.com/vimeo/psalm/tree/f1d47cc662500589e200a978e3af85488a5236cf/src/Psalm/Type * see https://github.com/phpDocumentor/ReflectionDocBlock/blob/622548b623e81ca6d78b721c5e029f4ce664f170/src/DocBlock/Tags/Param.php#L35 * see https://github.com/phpstan/phpdoc-parser/blob/d639142cf83e91a07b4862217a6f8aa65ee10d08/tests/PHPStan/Parser/TypeParserTest.php#L64 * /cc @tomasnorre this sadly effectively reverts #340 - it is too much effort to dive into further advanced types :-( * `PropertyTypeChanged` only expects the property to be both covariant and contravariant: if not, then a BC break was introduced (this re-uses some internal variance checks, simplifying code substantially) * usage of `ReflectionFunctionAbstract` is no longer present in the library: always using an union of `ReflectionMethod|ReflectionFunction` instead (as in `roave/better-reflection:^5`) * `FunctionName` is now `@internal`, since it's only used for internal rendering purposes
…-phpunit` This change ensures that we don't use `@internal` symbols in `src/` (too dangerous, for long-term usage), and makes the type checking much stricter, thanks to `psalm/plugin-phpunit`. Since `vimeo/psalm` has been upgraded, and vimeo/psalm#2772 no longer applies, we can also remove a lot of manually written code that was safe to clean up.
Ocramius
added a commit
to Roave/BetterReflection
that referenced
this pull request
Dec 5, 2021
…tionNamedType` Fixes #901 ## Context Currently, when running reflection on an `A|null` type, BetterReflection produces a `Roave\BetterReflection\Reflection\ReflectionUnionType`: ```php var_dump( get_class( (new DefaultReflector(new StringSourceLocator( <<<'PHP' <?php interface A {} final class AClass { private A|null $typed; } PHP , (new BetterReflection())->astLocator() ))) ->reflectClass('AClass') ->getProperty('typed') ->getType() ) ); ``` produces ``` string(53) "Roave\BetterReflection\Reflection\ReflectionUnionType" ``` In PHP-SRC, this behavior is different: https://3v4l.org/gMA4T#v8.1rc3 ```php <?php interface A {} interface B {} class Implementation { function foo(A|null $param) { throw new Exception(); } function bar(A|B|null $param) { throw new Exception(); } } var_dump((new ReflectionParameter([Implementation::class, 'foo'], 0))->getType()); var_dump((new ReflectionParameter([Implementation::class, 'bar'], 0))->getType()); ``` produces: ``` object(ReflectionNamedType)#2 (0) { } object(ReflectionUnionType)#1 (0) { } ``` This means that a `UnionType` AST node composed of just `null` plus another type should be converted into a `ReflectionNamedType`, for the sake of compatibility with upstream (this patch does that). This is ugly, but will (for now) avoid some bad issues in downstream handling (presently blocking Roave/BackwardCompatibilityCheck#324 )
Ocramius
added a commit
to Roave/BetterReflection
that referenced
this pull request
Dec 5, 2021
…tionNamedType` Fixes #901 ## Context Currently, when running reflection on an `A|null` type, BetterReflection produces a `Roave\BetterReflection\Reflection\ReflectionUnionType`: ```php var_dump( get_class( (new DefaultReflector(new StringSourceLocator( <<<'PHP' <?php interface A {} final class AClass { private A|null $typed; } PHP , (new BetterReflection())->astLocator() ))) ->reflectClass('AClass') ->getProperty('typed') ->getType() ) ); ``` produces ``` string(53) "Roave\BetterReflection\Reflection\ReflectionUnionType" ``` In PHP-SRC, this behavior is different: https://3v4l.org/gMA4T#v8.1rc3 ```php <?php interface A {} interface B {} class Implementation { function foo(A|null $param) { throw new Exception(); } function bar(A|B|null $param) { throw new Exception(); } } var_dump((new ReflectionParameter([Implementation::class, 'foo'], 0))->getType()); var_dump((new ReflectionParameter([Implementation::class, 'bar'], 0))->getType()); ``` produces: ``` object(ReflectionNamedType)#2 (0) { } object(ReflectionUnionType)#1 (0) { } ``` This means that a `UnionType` AST node composed of just `null` plus another type should be converted into a `ReflectionNamedType`, for the sake of compatibility with upstream (this patch does that). This is ugly, but will (for now) avoid some bad issues in downstream handling (presently blocking Roave/BackwardCompatibilityCheck#324 )
Ocramius
added a commit
to Roave/BetterReflection
that referenced
this pull request
Dec 5, 2021
…on types When an union type of `T|null` or `?T` is met, BetterReflection will (from now on) keep a `ReflectionUnionType` internally, while exposing a `ReflectionNamedType` with `ReflectionNamedType#allowsNull() === true` only at adapter level. While this is a BC break, it leads to a much cleaner API around handling `null` types, and inspecting types for type analysis. Ref: #902 (comment) Ref: Roave/BackwardCompatibilityCheck#324
This change strictly depends on Roave/BetterReflection#902, and once that's merged, new `infection/infection` mutations should appear, showing us code that can safely be removed.
… necessary with union types As per previous commit, `roave/better-reflection` has been upgraded so that nullable types are always represented as union types: this simplifies the logic massively, because, except for some edge cases, all nullability problems become a set intersection problem, rather than some ad-hoc conditionals. This further demonstrates that nullable types as union types is really valuable. Ref: https://twitter.com/Ocramius/status/1467392834558992390
|
@WyriHaximus since this is your favorite overall github project, can I have your review too, please? :D |
|
@Ocramius Sure, it seems that your CI is failing 😜 . Just kidding, let me go over the thing :) |
|
Haha, yeah, CI is failing because the |
WyriHaximus
approved these changes
Dec 5, 2021
There was a problem hiding this comment.
Not perse my overall favourite project but I'm using it for some cool stuff https://github.com/WyriHaximus/php-test-utilities/pull/260/files#diff-c8f1537d7020a410a1f19233721bb24de6aa93df4ef9b40795453c1647ed5f99R31 and some other stuff is blocked by it WyriHaximus/php-test-utilities#332
|
@Ocramius Ow yeah, I didn't even look at that. Just wanted to joke about it :P. Jokes aside, this is one reason why I want to use this package more: Make it comment on PR's about what BC would be introduced where. |
|
🚢 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This patch brings in PHP 8.0 and 8.1 support through the massive work done by @kukulich on
roave/better-reflectionover the past few months.The upgrade is mostly smooth, but we have some rough edges with:
TypeIsCovariant, which needsReflectionTypes as well asReflectorsTypeIsContravariant, which needsReflectionTypes as well asReflectorsReturnTypeCovarianceChangeduses hacky reflection to get aReflectorfrom aReflectionFunctionAbstract(internal state!) - needs AddedReflectionNamedType::getClass()BetterReflection#836ParameterTypeContravarianceChangeduses hacky reflection to get aReflectorfrom aReflectionFunctionAbstract(internal state!) - needs AddedReflectionNamedType::getClass()BetterReflection#836ReflectionUnionTypeandReflectionIntersectionType(infection/infectionwill highlight that)A|nullyield a nullableReflectionNamedTypeBetterReflection#902ReflectionIntersectionType(needs Ensure that simple union types likeA|nullyield a nullableReflectionNamedTypeBetterReflection#902 )Fixes #283