Skip to content

Conversation

@ndm2
Copy link
Contributor

@ndm2 ndm2 commented Jun 17, 2019

Install paths defined via the installed_paths configuration option do sometimes contain relative paths that may not exist depending on the current environment.

Resolving such non-existent paths causes false to be included in the results, which when used for looking up files/folders, can cause wrong locations to be looked up, and/or errors being thrown. \PHP_CodeSniffer\Util\Standards::getInstalledStandardDetails() would for example look for /ruleset.xml, ie in the root of the current drive, which could trigger an open basedir restriction error.

Non-existent relative paths resolve to `false`, including this in the
list of paths can cause errors when used for looking up files/folders.
@jrfnl
Copy link
Contributor

jrfnl commented Jun 18, 2019

IMO this is a valid fix.

I wonder: should a notice be shown to the user about unresolved paths to help debugging ?

@ndm2
Copy link
Contributor Author

ndm2 commented Jun 18, 2019

@jrfnl In my experience, I've mostly seen possibly non-existent paths when additional locations are being defined as a fallback. So I'm not sure, looking at for example getInstalledStandardDetails(), non-existent/accessible standard paths are deliberately silently stepped over, introducing notices would break with that... or are you maybe referring to output in verbose mode?

As far as I can tell, non existent paths would always finally trigger an error when a non existent sniff is referenced, one would then receive a message like:

ERROR: Referenced sniff "XYZ" does not exist

@ndm2 ndm2 changed the title Skip unresolvable relative standard paths. Skip unresolvable standard paths. Jun 19, 2019
@gsherwood gsherwood added this to the 3.5.0 milestone Sep 17, 2019
@gsherwood gsherwood changed the title Skip unresolvable standard paths. Unresolvable installed_paths can lead to open_basedir errors Sep 22, 2019
gsherwood added a commit that referenced this pull request Sep 22, 2019
@gsherwood gsherwood merged commit 6924b39 into squizlabs:master Sep 22, 2019
@gsherwood
Copy link
Member

Fix looked good to me too. Thanks a lot for finding and fixing 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.

3 participants