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

Syntax errors not being shown when error_prepend_string is set #1260

Merged
merged 1 commit into from
Jan 18, 2017

Conversation

jrfnl
Copy link
Contributor

@jrfnl jrfnl commented Jan 5, 2017

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";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@jrfnl jrfnl Jan 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

@aik099 aik099 Jan 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 force-pushed the feature/fix-php-syntax-sniff branch 2 times, most recently from b88befa to 32afa3a Compare January 5, 2017 16:13
Toggles the command for HHVM which does not have the `error_prepend_string` ini setting.
@gsherwood gsherwood changed the title Fix syntax errors not being shown when error_prepend_string was set. Syntax errors not being shown when error_prepend_string is set Jan 18, 2017
@gsherwood gsherwood merged commit 32afa3a into squizlabs:master Jan 18, 2017
gsherwood added a commit that referenced this pull request Jan 18, 2017
@gsherwood
Copy link
Member

Thanks a lot for fixing this.

@jrfnl jrfnl deleted the feature/fix-php-syntax-sniff branch January 18, 2017 05:15
@jrfnl
Copy link
Contributor Author

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants