Package home | Report new bug | New search | Development Roadmap Status: Open | Feedback | All | Closed Since Version 3.4.2

Bug #20196 1.5.2 breaks scope_closer position
Submitted: 2014-02-10 20:58 UTC
From: asgrim Assigned: squiz
Status: Closed Package: PHP_CodeSniffer (version 1.5.2)
PHP Version: 5.4.22 OS:
Roadmaps: (Not assigned)    
Subscription  


 [2014-02-10 20:58 UTC] asgrim (James Titcumb)
Description: ------------ I have a sniff that checks something with the scope_closer, but the scope_closer has changed between 1.5.1 and 1.5.2. The sniff registers for PHP_CodeSniffer_Tokens::$scopeOpeners occurances. If I run the sniff on the attached test script and var_dump($tokens[$stackPtr]); In 1.5.1: scope_closer is 36 (the endif;) In 1.5.2: scope_closer is 23 (the elseif...) Test script: --------------- <?php if ($foo): if ($bar) $foo = 1; elseif ($baz) $foo = 2; endif;

Comments

 [2014-02-11 10:37 UTC] squiz (Greg Sherwood)
-Status: Open +Status: Analyzed -Assigned To: +Assigned To: squiz
You are right that the behaviour changed. It was to fix a bug where this alternate syntax was not detecting else and elseif statements (bug #20151). In your example, the mixing of alternate syntax and inline syntax is causing a lot of confusion and I'm not really sure how to efficiently detect that. But it is clearly a problem so I'm going to keep looking into it.
 [2014-02-11 14:33 UTC] asgrim (James Titcumb)
Yes, you're quite right. I tried it with the following script and it works as expected: <?php if ($foo): if ($bar): $foo = 1; elseif ($baz): $foo = 2; endif; For us, at least, this is an acceptable fix in the short term, but ideally this should be fixed properly :)
 [2014-02-12 06:47 UTC] squiz (Greg Sherwood)
You're missing an endif; in that sample code and I think PHP_CodeSniffer is getting confused by nested IF statements using the alternate syntax. I'll try and fix that first, then looking into the mixed syntax problem after that.
 [2014-02-12 08:58 UTC] squiz (Greg Sherwood)
I fixed the problem with nested IFs using alternate syntax here: https://github.com/squizlabs/PHP_CodeSniffer/commit/46105c5973ae603855e9e710d 76899b65aecd898 Original issue is still not fixed, but I'm still not sure how to fix it yet either.
 [2014-02-12 14:30 UTC] asgrim (James Titcumb)
Ah yes, apologies, we did have an endif; as well, the actual code is quite big and includes lots of HTML, so I was paraphrasing! I install PHP_CodeSniffer by way of PEAR... could you point me to some docs to try out the in-development version at all?
 [2014-02-13 10:25 UTC] squiz (Greg Sherwood)
You can grab the latest code from Git and either install it via PEAR or just run it from the clone: To get the latest source and run from clone: git clone git://github.com/squizlabs/PHP_CodeSniffer.git cd PHP_CodeSniffer php scripts/phpcs -h Once you have the clone, you can install that via PEAR if you prefer: sudo pear uninstall PHP_CodeSniffer sudo pear install package.xml
 [2014-02-13 17:26 UTC] asgrim (James Titcumb)
Thanks Greg - I tried out the latest from master (71526ae7) and this fixes the problem, but I did notice an E_NOTICE (well, many of them).. I don't know if it's related: Notice: Undefined index: scope_condition On this "if": if ($tokens[$scopeEnd]['scope_condition'] !== $stackPtr) { return; } This only seems to happen when the scope ending token ($tokens[$scopeEnd]) is an "ELSE"... Not sure if this is intentional behaviour? The check is pretty much exactly the same check as here (https://github.com/squizlabs/PHP_CodeSniffer/blob/master/CodeSniffer/Standards/PEAR/Sniffs/WhiteSpace/ScopeClosingBraceSniff.php#L78) if that's any help?
 [2014-02-14 03:47 UTC] squiz (Greg Sherwood)
You must be testing code like this: if ($foo): else if ($baz): $foo = 2; endif; Where the ELSE and IF are separated, instead of ELSEIF. In this case, PHP_CodeSniffer sees that ELSE has to act as the end token for the IF due to the alternate syntax being used, but it never finds a scope opener for the ELSE because the next IF takes over and claims them for itself. While it makes sense what is happening to me, I suspect it makes no sense to anyone else :) So I'll see if I can restore the scope_condition index and make the ELSE appear to be a simple closer for the previous IF.
 [2014-02-14 03:56 UTC] squiz (Greg Sherwood)
Fixed that case I was talking about here: https://github.com/squizlabs/PHP_CodeSniffer/commit/2679192dfe5a3550ddf04a8e1b 86bcb5934158c1 Let me know if you have any more code that has this problem.
 [2014-02-14 14:48 UTC] asgrim (James Titcumb)
Hmm - it seems to have gone back to being broken now, I think perhaps the ELSE problem was masking it. The scope opener (the first "if") is still picking up the "elseif" as it's scope closer. The sniff is basically the PEAR.WhiteSpace.ScopeClosingBrace.Indent sniff, but modified slightly to use tabs instead of spaces. So I thought I'd change the code snippet I provided to spaces: <?php if ($foo): if ($bar) $foo = 1; elseif ($baz) $foo = 2; endif; and using the PEAR standard, sure enough I get the same error: 5 | ERROR | Closing brace indented incorrectly; expected 0 spaces, found 4 | | (PEAR.WhiteSpace.ScopeClosingBrace.Indent) Hopefully that will allow you to test a bit easier so we're on the same page? :) (there's a few other errors from the PEAR standard, but that's expected I think). Just to note, that it's only really on L119 of PEAR_Sniffs_WhiteSpace_ScopeClosingBraceSniff starts to differ significantly in our modified sniff - so the stuff that is determining where the scope_closer appears to be affecting that standard too I think.
 [2014-02-20 04:35 UTC] squiz (Greg Sherwood)
None of the changes I've made has re-broken anything for me. I haven't been able to fix the original issue, which is why your original code is still broken. The only way to write your statement so that PHPCS can detect it properly is like this: if ($foo): if ($bar): $foo = 1; elseif ($baz): $foo = 2; endif; endif; Because there is no ambiguity with the endif line, PHPCS can properly match then. This was not working originally when you report the bug either, but I have fixed this specific case.
 [2014-12-16 07:02 UTC] squiz (Greg Sherwood)
-Status: Analyzed +Status: Closed
I've finally fixed the original issue here: https://github.com/squizlabs/PHP_CodeSniffer/commit/6568a825c7731410f73cc54ec57 9eb923e9b6ed2 I had to add a special case for this syntax for if/elseif/else/endif, but the openers and closers are now correct.