8000 Suggestion: when detecting promisable by method presense check its an object by s-bronstein · Pull Request #161 · reactphp/promise · GitHub
[go: up one dir, main page]
Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Mar 5, 2020
Merged

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

merged 2 commits into from
Mar 5, 2020

Conversation

s-bronstein
Copy link
Contributor
@s-bronstein s-bronstein commented Mar 3, 2020 •
edited
Loading

Currently, detecting thenable causes unwanted side effects #160

@s-bronstein s-bronstein changed the title Suggestion: when detecting promisable my method presense check its an object Suggestion: when detecting promisable by method presense check its an object Mar 3, 2020
@jsor jsor requested review from WyriHaximus and clue March 5, 2020 11:36
clue
clue approved these changes Mar 5, 2020
View reviewed changes
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.

Changes LGTM, thanks for looking into this @smscr 👍

[…] If it is a string, and the contents of the string can be resolved to a class in the global namespace, and the class has these methods it can lead to unwanted behavior.

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

It looks like this applies the same fix like in #130, are there any other places that are affected by this?

@s-bronstein
Copy link
Contributor Author
s-bronstein commented Mar 5, 2020 •
edited
Loading

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
Copy link
Member
clue commented Mar 5, 2020

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

@WyriHaximus WyriHaximus merged commit ccd60f8 into reactphp:master Mar 5, 2020
@Seldaek
Copy link
Seldaek commented May 6, 2020

@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
Copy link
Member

@Seldaek sure will port it back into 1.x

@Seldaek
Copy link
Seldaek commented May 6, 2020

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
Copy link
Seldaek commented May 6, 2020

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

@clue clue added this to the v3.0.0 milestone May 6, 2020
@WyriHaximus
Copy link
Member

@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
< 8000 clipboard-copy aria-label="Copy link" for="issuecomment-624732878-permalink" role="menuitem" data-view-component="true" class="dropdown-item btn-link"> Copy link
Member
clue commented May 6, 2020

@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
Copy link
Seldaek commented May 6, 2020

@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
Copy link
Member

@clue @Seldaek cool thanks will have a look

@WyriHaximus WyriHaximus mentioned this pull request May 6, 2020
Merged
@WyriHaximus
Copy link
Member

@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 composer/composer#8878 but that's running into other issues

@ghost
Copy link
ghost commented May 6, 2020

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
Copy link
Seldaek commented May 8, 2020

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

@jsor jsor mentioned this pull request May 9, 2020
Merged
@jsor
Copy link
Member
jsor commented May 9, 2020

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 (e8c20be).

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 (#36).

So, we only need to backport what @CharlotteDunois suggests (#161 (comment)).

I've filed a PR for this: #168

@Seldaek
Copy link
Seldaek commented May 9, 2020

Thanks @jsor

@jsor
Copy link
Member
jsor commented May 12, 2020

@Seldaek Fix released in v2.8.0

@Seldaek
Copy link
Seldaek commented May 13, 2020

Thanks, seems to let it go through.

@clue clue added the bug label Jun 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jsor jsor jsor approved these changes

@WyriHaximus WyriHaximus WyriHaximus approved these changes

@clue clue clue approved these changes

Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
v3.0.0
Development

Successfully merging this pull request may close these issues.
5 participants
@s-bronstein @clue @Seldaek @WyriHaximus @jsor
0