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

Support union types and address deprecation of ReflectionType::getClass() (PHP 8+) #197

Merged
merged 3 commits into from
Oct 12, 2021

Conversation

SimonFrings
Copy link
Member

Big thank you to @cdosoftei for putting the time and effort into these changes!
He's done all the work in #176, I only rearranged some commits and added PHP 8 to the test matrix.

This pull request builds on top of #176, #195 and #196.
Resolves and Closes #176.

@bzikarsky
Copy link
Contributor

In response to #195's comment

@bzikarsky I added a new PR (#197). What do you think about this?

Obviously 👍 for a fix. I feel though that there is some depth in the discussion about intersection types in #195. It may make sense to look at the discussion/implementation and incroporate it into the final decision.

@clue
Copy link
Member

clue commented Oct 7, 2021

In response to #195's comment

@bzikarsky I added a new PR (#197). What do you think about this?

Obviously +1 for a fix. I feel though that there is some depth in the discussion about intersection types in #195. It may make sense to look at the discussion/implementation and incroporate it into the final decision.

As discussed in #197 I think both PRs add value, so I'm all for getting in both! :shipit: I would suggest we use this PR as a starting point as it adds support for union types as present as of PHP 8.0 and the required test suite. Once this is merged, perhaps we can update #197 to build on top of this and add additional tests for intersection types (future PHP 8.1+)?

Copy link
Member

@clue clue left a comment

Choose a reason for hiding this comment

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

@SimonFrings Thank you for taking this over and thank you @cdosoftei for the original changes!

The changes LGTM, let's get this in and then look into intersection types in a follow-up PR (see #195) :shipit:

@bzikarsky bzikarsky mentioned this pull request Oct 7, 2021
@bzikarsky
Copy link
Contributor

While I closed my simple change in #196 - this commit is also something that could proof useful for this MR: cc356b4

That the E_DEPRECATED were missed is partially caused by not testing on PHP8 but also by potentially not showing E_DEPRECATED in test-runs.

@WyriHaximus WyriHaximus merged commit d649e30 into reactphp:master Oct 12, 2021
@bzikarsky
Copy link
Contributor

bzikarsky commented Oct 12, 2021

I just realized: This got merged into master. Do you also backport this into 2.x? #195 targets 2.x.

Related: Do you want me to target master as well and rebase on top of it or wait and then rebase on 2.x?

@SimonFrings SimonFrings changed the title Add PHP 8 support and address deprecation of ReflectionType::getClass() Support union types and address deprecation of ReflectionType::getClass() Oct 15, 2021
@clue clue changed the title Support union types and address deprecation of ReflectionType::getClass() Support union types and address deprecation of ReflectionType::getClass() (PHP 8+) Nov 4, 2021
@SimonFrings SimonFrings mentioned this pull request Nov 15, 2021
@clue clue added this to the v3.0.0 milestone Feb 4, 2022
@clue
Copy link
Member

clue commented Feb 4, 2022

I just realized: This got merged into master. Do you also backport this into 2.x? #195 targets 2.x.

Related: Do you want me to target master as well and rebase on top of it or wait and then rebase on 2.x?

@bzikarsky I agree, this is indeed somewhat confusing. Here's the rundown:

I'll make sure to review your PR #195 and leave a comment to see how we can get this shipped asap :shipit:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants