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

Add explicit object checks before calling method_exists() #105

Merged
merged 1 commit into from Feb 12, 2020

Conversation

@jimcottrell
Copy link
Contributor

jimcottrell commented Oct 29, 2019

I'm sure this doesn't affect many people, but I came across this issue in an older project that uses an autoloader that in some circumstances can call include() without checking for file existence (relying on the class file existing somewhere in a directory defined in include_path).

Boiling it down, if you register a simple autoloader:

spl_autoload_register(function($class) {
	die($class);
});

Then any of the following will trigger the call to die:

$promise = new FulfilledPromise('NotAClassName1');
$promise = new RejectedPromise('NotAClassName2');
$promise = promise_for('NotAClassName3');
$promise = new Promise();
$promise->then(function() {}); // A handler must be defined to get to the relevant code
$promise->resolve('NotAClassName4'); // Or reject()

This happens because method_exists will attempt to load the class if provided with a string that could be a class name. So, this PR adds explicit object checks before calling method_exists, as I can't imagine triggering the autoloader being an intended side effect.

@GrahamCampbell GrahamCampbell mentioned this pull request Dec 31, 2019
@Tobion
Tobion approved these changes Feb 12, 2020
@@ -13,7 +13,7 @@ class FulfilledPromise implements PromiseInterface

public function __construct($value)
{
if (method_exists($value, 'then')) {
if (is_object($value) && method_exists($value, 'then')) {

This comment has been minimized.

Copy link
@Tobion

Tobion Feb 12, 2020

Member

The changes here look valid to me. But I wonder why we don't just check if ($value instanceof PromiseInterface)?

@mtdowling It seems the library was intentionally coded to work with some other promises. See for example also 1ef15a7
Michael, do you remember what other promises are meant to be supported? Was this before the introduction of the interface? Or what's the use-case? Can we remove this maybe legacy behavior?

This comment has been minimized.

Copy link
@mtdowling

mtdowling Feb 12, 2020

Member

IIRC, this is based on JS implementations and how promise libraries worked across implementations. This check allowed Guzzle promises to interop with React promises for example.

This comment has been minimized.

Copy link
@Tobion

Tobion Feb 12, 2020

Member

I see. I would think it's better to explicitly check instanceof for other interfaces or implemenations like https://github.com/reactphp/promise/blob/master/src/PromiseInterface.php
Just because there is a then method might not even mean the same thing or might not accept the same parameters at all.
But probably not worth to change this.

@Tobion Tobion merged commit 6379353 into guzzle:master Feb 12, 2020
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.