search

reactphp / promise

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

Support intersection types (PHP 8.1+ / Promise v2) #195

Closed bzikarsky closed 2 years ago

bzikarsky commented 3 years ago

PHP8's reflection API changed to make room for handling of union-types. As one of the consequences ReflectionParameter::getClass got 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.

bzikarsky commented 3 years ago

Needs to be squashed. I brainfarted and had a match in there. :zany_face: It's now a switch

SimonFrings commented 3 years ago

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

clue commented 3 years ago

@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! :+1:

bzikarsky commented 3 years ago

I'll rebase and add matching tests once the other MR is merged. :+1:

SimonFrings commented 2 years ago

@bzikarsky My PR is merged. :+1: Do you plan to pick this up again or do you want me to take a look at this? :shipit:

bzikarsky commented 2 years ago

Thanks for reminding me. I'll add it to this week's TODOs and hope that I don't get overwhelmed :-)

bzikarsky commented 2 years ago

Rebased and slightly adjusted.

SimonFrings commented 2 years ago

@bzikarsky Thanks for the update. 🔥 Can you add additional tests to ensure your changes work as expected?

bzikarsky commented 2 years ago

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.

bzikarsky commented 2 years ago

Pushed additional testcases in a separate commit for reviewability - Should be squashed before merge.

SimonFrings commented 2 years ago

@clue @WyriHaximus What are your thoughts on this?

peter17 commented 2 years ago

Hi! Can this be merged and 2.9.0 tagged? Thanks! Regards!

peter17 commented 2 years ago

@clue @jsor sorry to bother but can you please review? thanks a lot!

bzikarsky commented 2 years ago

Do you want to keep the refactoring of _checkTypehint and the feature+tests in two commits? Or do you prefer one?

Once we have this in, I'm happy to create a second PR with the changes applied to Promise v3.

clue commented 2 years ago

Do you want to keep the refactoring of _checkTypehint and the feature+tests in two commits? Or do you prefer one?

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 :+1:

Once we have this in, I'm happy to create a second PR with the changes applied to Promise v3.

Awesome! :+1:

bzikarsky commented 2 years ago

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! :+1: