Syntax errors not being shown when error_prepend_string is set #1260

Merged
merged 1 commit into from Jan 18, 2017

Projects

None yet

3 participants

@jrfnl
Contributor
jrfnl commented Jan 5, 2017 edited

Syntax errors were not being matched if the php.ini file used contained a value for error_prepend_string.

This fix makes double sure that those will now start showing up as it:

  • Overloads the ini value specifically for the php process used for the linting.
  • Matches the error line pattern per line in the result. The error_prepend_string will normally be on its own line in the output, so matching the error pattern for each line should prevent further issues.

Unit testing this would be near impossible as one would need to overload the php.ini file used by the linting process.

All the same, the issue can easily be reproduced:

  • Edit the php.ini file for the PHP version on which PHPCS is running, uncomment it and give it a value. Example often used by people mainly using PHP in the browser: <span style='color: #ff0000'> (combined with error_append_string = "</span>").
  • Run the sniff on a file with a parse error and see the sniff not throwing any errors.
  • Switch to this PR branch and run the sniff again. You should now see the parse error notification.

[Edit]: Recommitted to fix HHVM build failure.

@@ -74,11 +74,11 @@ public function process(PHP_CodeSniffer_File $phpcsFile, $stackPtr)
}
$fileName = $phpcsFile->getFilename();
- $cmd = $this->_phpPath." -l \"$fileName\" 2>&1";
+ $cmd = $this->_phpPath." -l -d error_prepend_string='' \"$fileName\" 2>&1";
@aik099
aik099 Jan 5, 2017 Contributor

You can clear the error_append_string as well while you're at it.

@jrfnl
jrfnl Jan 5, 2017 edited Contributor

The error_append_string won't cause any issues for this sniff as the regex does not use the $ for end of string/line. (which is why I didn't bother clearing it)

@aik099
aik099 Jan 5, 2017 edited Contributor

In that case I would rather change regex to figure out where errors are (instead of changing php.ini settings) in the text instead of matching from text start. That would be more error prone.

For example the on line ([0-9]+) part of regex uniquely identifies the error. This way we don't care where we find it and know for 100% that error text comes before it.

@jrfnl
jrfnl Jan 5, 2017 Contributor

The added m in the regex already makes it less error prone as it will match the ^ in the regex for start of line instead of start of string, so it will match as soon as it finds a line which contains a (parse) error message ;-)

That's what I meant by making "double sure" 😍

@jrfnl jrfnl Fix syntax errors not being shown when `error_prepend_string` was set.
Toggles the command for HHVM which does not have the `error_prepend_string` ini setting.
32afa3a
@jrfnl jrfnl referenced this pull request in WordPress-Coding-Standards/WordPress-Coding-Standards Jan 5, 2017
Closed

Notice: Undefined index: parenthesis_closer #248

@gsherwood gsherwood changed the title from Fix syntax errors not being shown when `error_prepend_string` was set. to Syntax errors not being shown when error_prepend_string is set Jan 18, 2017
@gsherwood gsherwood added the Bug label Jan 18, 2017
@gsherwood gsherwood merged commit 32afa3a into squizlabs:master Jan 18, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gsherwood gsherwood added a commit that referenced this pull request Jan 18, 2017
@gsherwood gsherwood Changelog for #1260 a75e015
@gsherwood
Member

Thanks a lot for fixing this.

@jrfnl jrfnl deleted the jrfnl:feature/fix-php-syntax-sniff branch Jan 18, 2017
@jrfnl
Contributor
jrfnl commented Jan 18, 2017

You're very welcome ;-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment