search

reactphp / promise

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

Suggestion: when detecting promisable by method presense check its an object #161

Closed s-bronstein closed 4 years ago

s-bronstein commented 4 years ago

Currently, detecting thenable causes unwanted side effects #160

s-bronstein commented 4 years ago

Can you be more specific what kind of "unwanted behavior" you're referring to? Perhaps we can (should?) add a test case for this?

@clue In my, case, a project has a class in global namespace which name was Index, and that name happened to be accidentially equal to the string value that was returned by a routine inside a Promise. The code was Promise\resolve'ing all returned values, so it got $promiseOrValue as an argument, it was a string, and the string was equal to 'Index', and there was a class in global namespace which name was Index. And it happened that the class had methods then and cancel which had no relation to Promises, of course.

It was a bit of too much coincidence, but two people spend a decent amout of hours with coffee trying to figure out what the hell happens there

clue commented 4 years ago

@smscr Now that's a nasty one, thanks for digging into this! :smile:

Seldaek commented 4 years ago

@WyriHaximus any chance of getting this fix backported on 1.x? It's breaking the Composer PHP 8 builds:

Uncaught TypeError: method_exists(): Argument #1 ($object_or_class) must be of type object|string, array given in /home/runner/work/composer/composer/vendor/react/promise/src/CancellationQueue.php:22
WyriHaximus commented 4 years ago

@Seldaek sure will port it back into 1.x

Seldaek commented 4 years ago

Thanks! I'm sure other stuff will break once that one is fixed but it's preventing us from running any tests at all :)

Seldaek commented 4 years ago

@WyriHaximus also if it can be backported to 2.x would probably make sense too.

WyriHaximus commented 4 years ago

@WyriHaximus any chance of getting this fix backported on 1.x? It's breaking the Composer PHP 8 builds:

Uncaught TypeError: method_exists(): Argument #1 ($object_or_class) must be of type object|string, array given in /home/runner/work/composer/composer/vendor/react/promise/src/CancellationQueue.php:22

So @Seldaek this is actually v2 erroring. Also are you passing [$object, 'method'] as callables around?

clue commented 4 years ago

@WyriHaximus This PR addresses the v3.x release branch, we've applied the same fix with #130 in v2.7.1 and have yet to backport this to the v1.x release branch.

Seldaek commented 4 years ago

@WyriHaximus you can see the details at https://github.com/composer/composer/runs/649345705?check_suite_focus=true incl stack trace. This is indeed running v2.7.1 but I'd like to make sure it works with 1.x on php 8 if possible.

I think most of our usage is via anonymous functions rather than array callbacks, but I might well be forgetting some.

WyriHaximus commented 4 years ago

@clue @Seldaek cool thanks will have a look

WyriHaximus commented 4 years ago

@Seldaek are you also testing locally and if so how? Attempting to get a PHP 8 docker image running locally but running into some issues, also filed https://github.com/composer/composer/pull/8878 but that's running into other issues

ghost commented 4 years ago

The second commit (which affects the CancellationQueue file) has not been backported to v2.x. Only the first commit was included in #130.

cc @clue @WyriHaximus

Seldaek commented 4 years ago

I did not test locally, just saw the failure on GH Actions sorry.

jsor commented 4 years ago

The failure happens here: https://github.com/reactphp/promise/blob/v2.7.1/src/functions.php#L146

In 2.x, we allow the $promisesOrValues argument to also be a promise which resolves to an array. That is changed for 3.x as we added the array type-declaration (e8c20be73335581a1069cdfb6113a946bb15a250).

In 2.x we rely on CancellationQueue::enqueue() just ignoring the call if it is an array and not a promise. So, the usage in Composer is correct as they call the function with an array.

1.x doesn't need any backport as it neither supports foreign promises in resolve() and also does not handle cancellation on promise collections (reactphp/promise#36).

So, we only need to backport what @CharlotteDunois suggests (https://github.com/reactphp/promise/pull/161#issuecomment-624839895).

I've filed a PR for this: https://github.com/reactphp/promise/pull/168

Seldaek commented 4 years ago

Thanks @jsor

jsor commented 4 years ago

@Seldaek Fix released in v2.8.0

Seldaek commented 4 years ago

Thanks, seems to let it go through.