search

reactphp / promise

Promises/A implementation for PHP.
https://reactphp.org/promise/
MIT License
2.38k stars 146 forks source link

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

Closed SimonFrings closed 3 years ago

SimonFrings commented 3 years ago

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 commented 3 years ago

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.

clue commented 3 years ago

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+)?

bzikarsky commented 3 years ago

While I closed my simple change in #196 - this commit is also something that could proof useful for this MR: https://github.com/reactphp/promise/pull/196/commits/cc356b4a3d668428e980fd31e9098755e4ee429a

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.

bzikarsky commented 3 years ago

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?

clue commented 2 years ago

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: