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

Fixed php-cs-fixer crashes on input file syntax error #5068

Merged
merged 1 commit into from
Oct 14, 2020

Conversation

GrahamCampbell
Copy link
Contributor

@GrahamCampbell GrahamCampbell commented Jul 25, 2020

  1. Some tokenizing lintier errors on PHP 7.0-7.2 cause the entire PHP process to fatal error. This was fixed in PHP 7.3, and so we should only use this linter on PHP 7.3 (catching the new type of error it can raise, instead of crashing).
  2. The above change highlighted the fact that some of the code samples in the tests are only valid on PHP 5, or are not valid at all. Those that are not valid on PHP 7, but are valid on PHP 5, I've updated to only run on PHP 5. Those that are not valid syntax, but can be corrected, I have corrected. Those that are just totally incorrect, have been removed.

Closes #5067.

@GrahamCampbell GrahamCampbell force-pushed the linter-fix branch 4 times, most recently from 4ad1f9b to a3cc021 Compare August 7, 2020 13:52
@GrahamCampbell GrahamCampbell force-pushed the linter-fix branch 3 times, most recently from 0c89140 to 93a42c0 Compare August 7, 2020 16:45
@GrahamCampbell GrahamCampbell changed the title Detect compile errors correctly Fixed php-cs-fixer crashes on input file syntax error Aug 7, 2020
@GrahamCampbell GrahamCampbell marked this pull request as ready for review August 7, 2020 17:07
@GrahamCampbell
Copy link
Contributor Author

Friendly ping @SpacePossum.

- Fixed invalid syntax of test cases
@SpacePossum SpacePossum added this to the 2.15.9 milestone Oct 14, 2020
@SpacePossum SpacePossum added the RTM Ready To Merge label Oct 14, 2020
@SpacePossum
Copy link
Contributor

SpacePossum commented Oct 14, 2020

Thank you @GrahamCampbell ! Great to see this fixed, lots of nice fixes, much appreciated 👍

Sorry for the wait, finally got a way to test on multiple PHP versions locally. FYI I've rebased and squashed your branch.

@SpacePossum SpacePossum removed the RTM Ready To Merge label Oct 14, 2020
@SpacePossum SpacePossum merged commit b6f3e4f into PHP-CS-Fixer:2.15 Oct 14, 2020
@GrahamCampbell GrahamCampbell deleted the linter-fix branch October 14, 2020 10:15
@GrahamCampbell
Copy link
Contributor Author

Did you remove the extra spaces in the error message? I added them to match the format from the native linter, which has two spaces after the colon.

@SpacePossum
Copy link
Contributor

I did indeed, however all tests did pass on the CI and local, you think this is going to be an issue somewhere?

@GrahamCampbell
Copy link
Contributor Author

I just left it in for consistency. Maybe we could remove the double spaces from the native linter?

@SpacePossum
Copy link
Contributor

Sounds good, we don't have a BC promise on exception messages anyway. Can you PR it or maybe link the related code than I can do it later on?

@GrahamCampbell
Copy link
Contributor Author

Oh, you changed the message prefix too? I specifically had it so it matched the native linter output.

@GrahamCampbell
Copy link
Contributor Author

#5174 should address the issues, ensuring consistent error messages.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants