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

Stabilize a condition #127

Merged
merged 1 commit into from
Dec 8, 2020
Merged

Conversation

jrfnl
Copy link
Member

@jrfnl jrfnl commented Dec 7, 2020

Proposed Changes

While looking into the (non-)race condition described in #125, I noticed a potentially related issue.

As per the documentation of realpath():

realpath() returns FALSE on failure, e.g. if the file does not exist.

In the rare case that realpath() would return false within this plugin, false would subsequently be passed on to is_dir() and is_readable() which both expect a string path.

This tiny change makes the condition fail earlier by checking the return value of realpath().

While looking into the race condition described in 125, I noticed a potentially related issue.

As per the documentation of [realpath()](https://www.php.net/manual/en/function.realpath.php#refsect1-function.realpath-returnvalues):
> realpath() returns FALSE on failure, e.g. if the file does not exist.

In the rare case that `realpath()` _would_ return `false` within this plugin, `false` would subsequently be passed on to `is_dir()` and `is_readable()` which both expect a string path.

This tiny change make the condition fail earlier by checking the return value of `realpath()`.
@jrfnl
Copy link
Member Author

jrfnl commented Dec 8, 2020

@Potherca Just checking: where there any changed you wanted ? Or did you assign this to me to self-merge ?
(sorry, the commit process in this repo still confuses me - I'm used to never self-merge when collaborating, i.e. the (last) approver merges)

@Potherca Potherca merged commit c4a47f7 into master Dec 8, 2020
@Potherca Potherca deleted the feature/realpath-can-return-false branch December 8, 2020 15:46
@Potherca
Copy link
Member

Potherca commented Dec 8, 2020

No changes needed. I just didn't feel like waiting around for Travis to complete but also didn't want to use my admin right to merge again. I assigned you in case I forgot to come back to this 😂

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.

2 participants