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

Squiz.Strings.EchoedStrings does not properly fix bracketed statements #1150

Closed
u01jmg3 opened this issue Sep 8, 2016 · 4 comments
Closed

Comments

@u01jmg3
Copy link

u01jmg3 commented Sep 8, 2016

Using the following rules and PHP, PHPCBF makes an error during correction.

<rule ref="Squiz.Strings.EchoedStrings" />
<rule ref="Squiz.WhiteSpace.SemicolonSpacing" />
echo($y) ;

// becomes...

echo $y);

Modifying EchoedStringsSniff.php#L67 and including T_WHITESPACE solves the problem.

// with code extended

echo $y;
@gsherwood
Copy link
Member

Debug output from --report=diff -vvv:

*** START FILE FIXING ***
---START FILE CONTENT---
1|<?php
2|echo($y) ;
3|
--- END FILE CONTENT ---
        E: [Line 2] Echoed strings should not be bracketed (Squiz.Strings.EchoedStrings.HasBracket)
        => Changeset started by Squiz_Sniffs_Strings_EchoedStringsSniff (line 89)
            Q: Squiz_Sniffs_Strings_EchoedStringsSniff (line 90) replaced token 2 (T_OPEN_PARENTHESIS) "(" => ""
            Q: Squiz_Sniffs_Strings_EchoedStringsSniff (line 91) replaced token 5 (T_WHITESPACE) "·;" => ";"
            A: Squiz_Sniffs_Strings_EchoedStringsSniff (line 92) replaced token 2 (T_OPEN_PARENTHESIS) "(" => ""
            A: Squiz_Sniffs_Strings_EchoedStringsSniff (line 92) replaced token 5 (T_WHITESPACE) "·;" => ";"
        => Changeset ended: 2 changes applied
        E: [Line 2] Space found before semicolon; expected ");" but found ") ;" (Squiz.WhiteSpace.SemicolonSpacing.Incorrect)
        => Changeset started by Squiz_Sniffs_WhiteSpace_SemicolonSpacingSniff (line 90)
            Q: Squiz_Sniffs_WhiteSpace_SemicolonSpacingSniff (line 92) replaced token 5 (T_WHITESPACE) ";" => ";"
            * token 5 has already been modified, skipping *
        => Changeset failed to apply
        * fixed 2 violations, starting loop 2 *
---START FILE CONTENT---
1|<?php
2|echo$y);
3|
--- END FILE CONTENT ---
        E: [Line 2] Language constructs must be followed by a single space; expected "echo $y" but found "echo$y" (Squiz.WhiteSpace.LanguageConstructSpacing.Incorrect)
        Squiz_Sniffs_WhiteSpace_LanguageConstructSpacingSniff (line 93) replaced token 1 (T_ECHO) "echo" => "echo·"
        * fixed 1 violations, starting loop 3 *
---START FILE CONTENT---
1|<?php
2|echo $y);
3|
--- END FILE CONTENT ---
*** END FILE FIXING ***

@gsherwood gsherwood changed the title Extend EchoedStringsSniff to cover spacing scenario Squiz.Strings.EchoedStrings does not properly fix bracketed statements Sep 12, 2016
gsherwood added a commit that referenced this issue Sep 12, 2016
@gsherwood
Copy link
Member

Thanks for the bug report. I found another issue with the fixer that I also corrected (open brace wasn't given a space before it when needed).

@u01jmg3
Copy link
Author

u01jmg3 commented Nov 13, 2016

Will this be included in v3.0.1?

@gsherwood
Copy link
Member

Will this be included in v3.0.1?

3.0.1 is not the next 3.x release (3.0.0 isn't out yet), but it will be in both the next 2.x and 3.x releases.

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