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

Fix file stat for cases when file doesn't exist #19

Merged
merged 1 commit into from
Mar 14, 2018

Conversation

seregazhuk
Copy link
Contributor

When calling exists() or stat() on file object and a specified file doesn't exists the code fails. The reason is that everywhere in React\Filesystem\WoolTrait for rejecting promises is being used error_get_last() function. But when checking for existence you specify a custom array that has some different structure than the result of error_get_last(). In this case here it fails:

https://github.com/reactphp/filesystem/blob/master/src/ChildProcess/Adapter.php#L134

because there is no such key as message.

@clue
Copy link
Member

clue commented Mar 13, 2018

@seregazhuk Thanks for reporting! I'm not too familiar with this particular code, can we add a test for this? 👍

@seregazhuk
Copy link
Contributor Author

@clue Done! I was a bit confused because the test for this use case already exists. But it only checks that the promise rejects. So, when the code fails, the promise rejects as it was expected in the test but with Illegal string offset 'message' message. I've updated the test to check the rejection reason of the promise.

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.

Thank you for the quick update, changes LGTM! 👍

@WyriHaximus Do you know what's up with the failing test cases?

@WyriHaximus
Copy link
Member

@clue yeah it is an functional test that fails (works on my machine) so I'm working on it in my labs fork to find out how to fix it.

@WyriHaximus WyriHaximus merged commit 1e2311a into reactphp:master Mar 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants