Skip to content

PHPCBF can make invalid fixes to inline JS control structures that make use of JS objects #1432

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

Closed
conantran opened this issue Apr 26, 2017 · 4 comments

Comments

@conantran
Copy link

Hello,

I ran phpcbf on the following javascript code: (notice the use of short hand, no open and close brackets which is violating psr2

if ($("#myid").rotationDegrees()=='90')
    $('.modal').css({'transform': 'rotate(90deg)'});

and it modify the above as: (notice how phpcbf add open bracket in the if control statement but failed to add the closing bracket, also it removed bracket in the .cssline

if ($("#myid").rotationDegrees()=='90') {
    $('.modal').css('transform': 'rotate(90deg)'});

which produced an javascript error exception in browser. However, if I ran phpcbf on the following (added open and close brackets and spaces)

if ($("#myid").rotationDegrees()=='90') {
    $('.modal').css(   {'transform': 'rotate(90deg)'});
}

the tools run ok.

I think this is not expected, I would expect the phpcbf to fix the above by adding opening and closing bracket in the control structure but not modifying what within it.

@gsherwood
Copy link
Member

The problem is that the tokenizer thinks the first { is the opening curly brace for the if. If it didn't, it would correctly report this as an inline control structure and a different sniff would find and fix that.

@conantran
Copy link
Author

thanks @gsherwood, by saying a different sniff would find and fix that, what do you mean by that?

this broke my javascript and after the sniff try to fix the code style though so i'm not at all 100% confident to run the auto code fix now.

@gsherwood
Copy link
Member

by saying a different sniff would find and fix that, what do you mean by that?

The sniff that detects and fixes inline control structures would find and fix this. The sniff that is causing it to break would ignore this code because the control structure doesn't have any braces.

This is the code I have been testing:

if ($("#myid").rotationDegrees()=='90')
    $('.modal').css({'transform': 'rotate(90deg)'});

if ($("#myid").rotationDegrees()=='90')
    $foo = {'transform': 'rotate(90deg)'};

And PHPCS will report these errors (note the sniff codes):

FILE: temp.js
----------------------------------------------------------------------
FOUND 4 ERRORS AFFECTING 4 LINES
----------------------------------------------------------------------
 1 | ERROR | [x] Expected 1 space after closing parenthesis; found "\n
   |       |     $('.modal').css("
   |       |     (Squiz.ControlStructures.ControlSignature.SpaceAfterCloseParenthesis)
 2 | ERROR | [x] Newline required after opening brace
   |       |     (Squiz.ControlStructures.ControlSignature.NewlineAfterOpenBrace)
 4 | ERROR | [x] Expected 1 space after closing parenthesis; found "\n
   |       |     $foo = "
   |       |     (Squiz.ControlStructures.ControlSignature.SpaceAfterCloseParenthesis)
 5 | ERROR | [x] Newline required after opening brace
   |       |     (Squiz.ControlStructures.ControlSignature.NewlineAfterOpenBrace)
----------------------------------------------------------------------
PHPCBF CAN FIX THE 4 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

After fixing the tokenizer, you get these errors instead:

FILE: temp.js
--------------------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
--------------------------------------------------------------------------------
 1 | ERROR | [x] Inline control structures are not allowed
   |       |     (Generic.ControlStructures.InlineControlStructure.NotAllowed)
 4 | ERROR | [x] Inline control structures are not allowed
   |       |     (Generic.ControlStructures.InlineControlStructure.NotAllowed)
--------------------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------------

And now PHPCBF fixes the file correctly by adding braces:

--- temp.js
+++ PHP_CodeSniffer
@@ -1,5 +1,7 @@
-if ($("#myid").rotationDegrees()=='90')
+if ($("#myid").rotationDegrees()=='90') {
     $('.modal').css({'transform': 'rotate(90deg)'});
+}

-if ($("#myid").rotationDegrees()=='90')
+if ($("#myid").rotationDegrees()=='90') {
     $foo = {'transform': 'rotate(90deg)'};
+}

NOTE that this assumes your standard has included the Generic.ControlStructures.InlineControlStructure sniff. If it doesn't include this sniff, PHPCBF will not make any changes to your code.

@gsherwood gsherwood changed the title phpcbf cause issue on control structure without open and close brackets PHPCBF can make invalid fixes to inline JS control structures that make use of JS objects Apr 27, 2017
gsherwood added a commit that referenced this issue Apr 27, 2017
@gsherwood
Copy link
Member

I've pushed up a fix for this issue, to be included in the next 2.x and 3.x releases. Thanks for the report.

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

No branches or pull requests

2 participants