Fix file_exists warning when resolving with long strings #130
Conversation
clue
left a comment
clue
left a comment
There was a problem hiding this comment.
@sbesselsen Good catch, that's a nasty one! 🥇
Does it make sense to add a test for this? 👍
|
@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 👍 |
|
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 |
|
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? |
WyriHaximus
left a comment
WyriHaximus
left a comment
There was a problem hiding this comment.
LGTM 👍 . And while I really like to have a test for this I see no reason to block this, we can do the test in a follow up PR and put this out there in the mean time.
4 participants
When resolving a promise with a long string, the following may happen:
The method_exists() check only makes sense for objects, since further down the call
$promiseOrValue->then(...)is made. Staticthenmethods aren't supported anyway.