Support intersection types (PHP 8.1+ / Promise v2) #195
Conversation
This comment has been minimized.
This comment has been minimized.
bzikarsky
force-pushed
the
patch-1
branch
2 times, most recently
from
b7eb5e6
to
22d147b
Compare
bzikarsky
changed the title
|
@bzikarsky I added a new PR (#197). What do you think about this? |
|
@bzikarsky Thank you for filing this PR, some very good changes in here! I think the original PR filed in #176 by @cdosoftei and rebased in #197 by @SimonFrings is an excellent starting point for PHP 8 support as it also adds additional tests to ensure this behavior is consistent across versions. Perhaps we can get this in first and then rebase your work with regards to intersection types in PHP 8.1 on top of this PR with some additional tests? Either way, keep it up! 👍 |
|
I'll rebase and add matching tests once the other MR is merged. 👍 |
|
@bzikarsky My PR is merged. 👍 |
|
Thanks for reminding me. I'll add it to this week's TODOs and hope that I don't get overwhelmed :-) |
|
Rebased and slightly adjusted. |
|
@bzikarsky Thanks for the update. 🔥 |
|
Good point. Yes, of course. There will only be an 8.1-only test for the intersection types though. For everything else it is just a refactoring. |
|
Pushed additional testcases in a separate commit for reviewability - Should be squashed before merge. |
|
@clue @WyriHaximus What are your thoughts on this? |
There was a problem hiding this comment.
Aside from the nits @SimonFrings mentioned this looks good to me 👍
|
Hi! Can this be merged and 2.9.0 tagged? Thanks! Regards! |
clue
changed the title
There was a problem hiding this comment.
@bzikarsky Thank you very much for much for keeping up with this and filing this high-quality PR! 👍 Functionally, the changes LGTM and I'd like to get this in as-is 
As discussed in this ticket and linked tickets, this PR has evolved quite a bit since it was originally filed and parts of the original functionality have already been merged via other PRs. Here's the rundown:
- PR Support union types and address deprecation of
ReflectionType::getClass()(PHP 8+) #197 adds support for union types (PHP 8+) for Promise v3 (by rebasing the changes originally from Address deprecation of ReflectionType::getClass() #176) - PR Support union types and address deprecation of
ReflectionType::getClass()(PHP 8+ / Promise v2) #198 adds support for union types (PHP 8+) for Promise v2 (by backporting PR Support union types and address deprecation ofReflectionType::getClass()(PHP 8+) #197) - This PR Support intersection types (PHP 8.1+ / Promise v2) #195 adds support for intersection types (PHP 8.1+) for Promise v2 (by building on top of Support union types and address deprecation of
ReflectionType::getClass()(PHP 8+) #197 and Support PHP 8.1 (Promise v2) #200) - There's indeed currently no PR to add support for intersection types (PHP 8.1+) for Promise v3 (possibly by cherry-picking your changes from this PR Support intersection types (PHP 8.1+ / Promise v2) #195)
Sorry for the confusion above! I've just updated the PR title to reflect what this PR (now) does. If you can squash your changes (and update the commit message) to reflect this, I'm happy to get this in as-is
(For the reference: Once this is in, we still need to port this to Promise v3. We don't have a strong policy in place about this, but for all other PRs we've gone the opposite route of first merging for Promise v3 and then backporting to v2, but either approach works for me.)
Thanks for your excellent work, keep it up! 👍
|
Do you want to keep the refactoring of Once we have this in, I'm happy to create a second PR with the changes applied to Promise v3. |
I would argue that each functional changes should come with additional tests as part of the same commit. Leaving the refactoring in a separate commit sounds reasonable, but I'll leave this up to you to decide 👍
Awesome! 👍 |
PHP8's reflection API changed to make room for handling of union-types. As one of the consequences `ReflectionParameter::getClass` got marked as deprecated. While the bug got already fixed in the meantime, the proposed code clears up the code-path for future features like PHP8.1's intersection-types.
|
I prefer the separation of refactoring and feature as well. Tests are squashed into the feature. had to rebase once more though. From my perspective: Good to go! 👍 |
There was a problem hiding this comment.
@bzikarsky Thank you for this high-quality PR, changes LGTM! 👍
PHP8's reflection API changed to make room for handling of union-types. As one of the consequences
ReflectionParameter::getClassgot marked as deprecated.This commit moves the legacy path behind a version-check for PHP versions before 8 and adds a new more complex path for PHP8+.
Fixes #192.