Fix file_exists warning when resolving with long strings

reactphp / promise

Promises/A implementation for PHP.

https://reactphp.org/promise/

MIT License

2.38k stars 146 forks source link

Fix file_exists warning when resolving with long strings #130

Closed sbesselsen closed 5 years ago

sbesselsen commented 5 years ago

When resolving a promise with a long string, the following may happen:

method_exists is called
This triggers the class autoloader to check for a class by that name
The class autoloader will use file_exists to check for a file by that name
file_exists emits a warning: Warning: file_exists(): File name is longer than the maximum allowed path length on this platform ([...])

The method_exists() check only makes sense for objects, since further down the call $promiseOrValue->then(...) is made. Static then methods aren't supported anyway.

WyriHaximus commented 5 years ago

@clue I think it makes sense to add a test. Not sure how we'd test this yet to be honest. But if @sbesselsen has an idea how to test this that would be great 👍

jsor commented 5 years ago

I'm also not sure how to test this. For me the patch is good as is. My only suggestion is to add a comment for the reasoning behind the is_object() check.

sbesselsen commented 5 years ago

Hi all, thanks for the feedback. I've added a comment as requested.

I've also thought about a good way to test it; the one thing I could think of was to create a test setup where we have a class with a method "then", and run resolve() with that class name. We would expect that promise to then resolve with the class name, whereas before we would get a PHP error "Call to a member function then() on string". Does that sound like a good test? I'm unsure because it would require us to define a class in the test script just for that purpose, which seems... strange?