Bug #20151 :: Problem handling "if(): ... else: ... endif;" syntax

Package home | Report new bug | New search | Development Roadmap

Status: Open | Feedback | All | Closed Since Version 3.7.2

Bug #20151	Problem handling "if(): ... else: ... endif;" syntax
Submitted:	2013-12-18 22:03 UTC
From:	yannicmahe	Assigned:	squiz
Status:	Closed	Package:	PHP_CodeSniffer (version 1.5.1)
PHP Version:	Irrelevant	OS:	Irrelevant
Roadmaps:	(Not assigned)
Subscription	Your email:

Comments Patches (2) Add Comment Add patch

[2013-12-18 22:03 UTC] yannicmahe (Yannic Mahe)

Description:
------------
I had some troubles in analysing some smarty cache files. Indeed, it was taking ages for somes files. For instance, I stopped the analysis of a file after 8 hours running...

After some digging, it appears that the problem is around the handling done for the PHP special if/else/endif syntax :
if(...):
else:
endif;

In fact, it's totally broken in the current version :
- the ELSE scope is never effectively found
- to find the IF scope, the whole file is analysed. Which leads to lot of processing when there are multiple if/else imbricated or in a row

I may have a way to fix it by doing the following things in PHP\CodeSniffer\Tokenizers\PHP.php :
=> define T_COLON as a starting token for T_ELSE and T_ELSEIF
=> define T_ENDIF as a closing token for T_ELSE AND T_ELSEIF
=> define T_IF, T_ELSE and T_ELSEIF as sharing their closing token together

After doing this change (I put the result in the "Expected result" field"), the analysis seems correct.
Of course I tests the fix against the file that was taking more than 8 hours to analyse, and it took about 1s to process it !

I don't thing the fix will impact the standard if/else syntax analysis, but it should be checked before being applied.

Test script:
---------------
<?php if ($test): ?>
	Something
<?php else: ?>
	Something else
<?php endif;?>
	Something that shouldn't be analyzed for the scopes of the if/else
<?php if ($anothertest): ?>
	another thing
<?php else: ?>
	another something else
<?php endif; ?>


Expected result:
----------------
	*** START SCOPE MAP ***
	Start scope map at 1: T_IF => if
	Process token 2 []: T_WHITESPACE =>  
	Process token 3 []: T_OPEN_PARENTHESIS => (
	* skipping parenthesis *
	Process token 6 []: T_COLON => :
	=> Found scope opener for 1 (T_IF)
	Process token 7 [opener:6;]: T_WHITESPACE =>  
	Process token 8 [opener:6;]: T_CLOSE_TAG => ?>\n
	Process token 9 [opener:6;]: T_INLINE_HTML => 	Something\n
	Process token 10 [opener:6;]: T_OPEN_TAG => <?php 
	Process token 11 [opener:6;]: T_ELSE => else
	* token is an opening condition *
	* searching for opener *
		Process token 12 []: T_COLON => :
		=> Found scope opener for 11 (T_ELSE)
		Process token 13 [opener:12;]: T_WHITESPACE =>  
		Process token 14 [opener:12;]: T_CLOSE_TAG => ?>\n
		Process token 15 [opener:12;]: T_INLINE_HTML => 	Something else\n
		Process token 16 [opener:12;]: T_OPEN_TAG => <?php 
		Process token 17 [opener:12;]: T_ENDIF => endif
		=> Found scope closer for 11 (T_ELSE)
	Process token 13 [opener:6;]: T_WHITESPACE =>  
	Process token 14 [opener:6;]: T_CLOSE_TAG => ?>\n
	Process token 15 [opener:6;]: T_INLINE_HTML => 	Something else\n
	Process token 16 [opener:6;]: T_OPEN_TAG => <?php 
	Process token 17 [opener:6;]: T_ENDIF => endif
	=> Found scope closer for 1 (T_IF)
	Start scope map at 11: T_ELSE => else
	Process token 12 []: T_COLON => :
	=> Found scope opener for 11 (T_ELSE)
	Process token 13 [opener:12;]: T_WHITESPACE =>  
	Process token 14 [opener:12;]: T_CLOSE_TAG => ?>\n
	Process token 15 [opener:12;]: T_INLINE_HTML => 	Something else\n
	Process token 16 [opener:12;]: T_OPEN_TAG => <?php 
	Process token 17 [opener:12;]: T_ENDIF => endif
	=> Found scope closer for 11 (T_ELSE)
	Start scope map at 22: T_IF => if
	Process token 23 []: T_WHITESPACE =>  
	Process token 24 []: T_OPEN_PARENTHESIS => (
	* skipping parenthesis *
	Process token 27 []: T_COLON => :
	=> Found scope opener for 22 (T_IF)
	Process token 28 [opener:27;]: T_WHITESPACE =>  
	Process token 29 [opener:27;]: T_CLOSE_TAG => ?>\n
	Process token 30 [opener:27;]: T_INLINE_HTML => 	another thing\n
	Process token 31 [opener:27;]: T_OPEN_TAG => <?php 
	Process token 32 [opener:27;]: T_ELSE => else
	* token is an opening condition *
	* searching for opener *
		Process token 33 []: T_COLON => :
		=> Found scope opener for 32 (T_ELSE)
		Process token 34 [opener:33;]: T_WHITESPACE =>  
		Process token 35 [opener:33;]: T_CLOSE_TAG => ?>\n
		Process token 36 [opener:33;]: T_INLINE_HTML => 	another something else\n
		Process token 37 [opener:33;]: T_OPEN_TAG => <?php 
		Process token 38 [opener:33;]: T_ENDIF => endif
		=> Found scope closer for 32 (T_ELSE)
	Process token 34 [opener:27;]: T_WHITESPACE =>  
	Process token 35 [opener:27;]: T_CLOSE_TAG => ?>\n
	Process token 36 [opener:27;]: T_INLINE_HTML => 	another something else\n
	Process token 37 [opener:27;]: T_OPEN_TAG => <?php 
	Process token 38 [opener:27;]: T_ENDIF => endif
	=> Found scope closer for 22 (T_IF)
	Start scope map at 32: T_ELSE => else
	Process token 33 []: T_COLON => :
	=> Found scope opener for 32 (T_ELSE)
	Process token 34 [opener:33;]: T_WHITESPACE =>  
	Process token 35 [opener:33;]: T_CLOSE_TAG => ?>\n
	Process token 36 [opener:33;]: T_INLINE_HTML => 	another something else\n
	Process token 37 [opener:33;]: T_OPEN_TAG => <?php 
	Process token 38 [opener:33;]: T_ENDIF => endif
	=> Found scope closer for 32 (T_ELSE)
	*** END SCOPE MAP ***
	*** START LEVEL MAP ***
	Process token 0 on line 1 [lvl:0;]: T_OPEN_TAG => <?php 
	Process token 1 on line 1 [lvl:0;]: T_IF => if
	Process token 2 on line 1 [lvl:0;]: T_WHITESPACE =>  
	Process token 3 on line 1 [lvl:0;]: T_OPEN_PARENTHESIS => (
	Process token 4 on line 1 [lvl:0;]: T_VARIABLE => $test
	Process token 5 on line 1 [lvl:0;]: T_CLOSE_PARENTHESIS => )
	Process token 6 on line 1 [lvl:0;]: T_COLON => :
	=> Found scope opener for 1 (T_IF)
		* level increased *
		* token 1 (T_IF) added to conditions array *
		Process token 7 on line 1 [lvl:1;conds;T_IF;]: T_WHITESPACE =>  
		Process token 8 on line 1 [lvl:1;conds;T_IF;]: T_CLOSE_TAG => ?>\n
		Process token 9 on line 2 [lvl:1;conds;T_IF;]: T_INLINE_HTML => 	Something\n
		Process token 10 on line 3 [lvl:1;conds;T_IF;]: T_OPEN_TAG => <?php 
		Process token 11 on line 3 [lvl:1;conds;T_IF;]: T_ELSE => else
		Process token 12 on line 3 [lvl:1;conds;T_IF;]: T_COLON => :
		=> Found scope opener for 11 (T_ELSE)
			* level increased *
			* token 11 (T_ELSE) added to conditions array *
			Process token 13 on line 3 [lvl:2;conds;T_IF,T_ELSE;]: T_WHITESPACE =>  
			Process token 14 on line 3 [lvl:2;conds;T_IF,T_ELSE;]: T_CLOSE_TAG => ?>\n
			Process token 15 on line 4 [lvl:2;conds;T_IF,T_ELSE;]: T_INLINE_HTML => 	Something else\n
			Process token 16 on line 5 [lvl:2;conds;T_IF,T_ELSE;]: T_OPEN_TAG => <?php 
			Process token 17 on line 5 [lvl:2;conds;T_IF,T_ELSE;]: T_ENDIF => endif
			=> Found scope closer for 12 (T_COLON)
			* token T_ELSE removed from conditions array *
			* level decreased *
		=> Found scope closer for 6 (T_COLON)
		* token T_IF removed from conditions array *
		* level decreased *
	Process token 18 on line 5 [lvl:0;]: T_SEMICOLON => ;
	Process token 19 on line 5 [lvl:0;]: T_CLOSE_TAG => ?>\n
	Process token 20 on line 6 [lvl:0;]: T_INLINE_HTML => 	Something that shouldn't be analyzed for the scopes of the if/else\n
	Process token 21 on line 7 [lvl:0;]: T_OPEN_TAG => <?php 
	Process token 22 on line 7 [lvl:0;]: T_IF => if
	Process token 23 on line 7 [lvl:0;]: T_WHITESPACE =>  
	Process token 24 on line 7 [lvl:0;]: T_OPEN_PARENTHESIS => (
	Process token 25 on line 7 [lvl:0;]: T_VARIABLE => $anothertest
	Process token 26 on line 7 [lvl:0;]: T_CLOSE_PARENTHESIS => )
	Process token 27 on line 7 [lvl:0;]: T_COLON => :
	=> Found scope opener for 22 (T_IF)
		* level increased *
		* token 22 (T_IF) added to conditions array *
		Process token 28 on line 7 [lvl:1;conds;T_IF;]: T_WHITESPACE =>  
		Process token 29 on line 7 [lvl:1;conds;T_IF;]: T_CLOSE_TAG => ?>\n
		Process token 30 on line 8 [lvl:1;conds;T_IF;]: T_INLINE_HTML => 	another thing\n
		Process token 31 on line 9 [lvl:1;conds;T_IF;]: T_OPEN_TAG => <?php 
		Process token 32 on line 9 [lvl:1;conds;T_IF;]: T_ELSE => else
		Process token 33 on line 9 [lvl:1;conds;T_IF;]: T_COLON => :
		=> Found scope opener for 32 (T_ELSE)
			* level increased *
			* token 32 (T_ELSE) added to conditions array *
			Process token 34 on line 9 [lvl:2;conds;T_IF,T_ELSE;]: T_WHITESPACE =>  
			Process token 35 on line 9 [lvl:2;conds;T_IF,T_ELSE;]: T_CLOSE_TAG => ?>\n
			Process token 36 on line 10 [lvl:2;conds;T_IF,T_ELSE;]: T_INLINE_HTML => 	another something else\n
			Process token 37 on line 11 [lvl:2;conds;T_IF,T_ELSE;]: T_OPEN_TAG => <?php 
			Process token 38 on line 11 [lvl:2;conds;T_IF,T_ELSE;]: T_ENDIF => endif
			=> Found scope closer for 33 (T_COLON)
			* token T_ELSE removed from conditions array *
			* level decreased *
		=> Found scope closer for 27 (T_COLON)
		* token T_IF removed from conditions array *
		* level decreased *
	Process token 39 on line 11 [lvl:0;]: T_SEMICOLON => ;
	Process token 40 on line 11 [lvl:0;]: T_WHITESPACE =>  
	Process token 41 on line 11 [lvl:0;]: T_CLOSE_TAG => ?>\n
	*** END LEVEL MAP ***


Actual result:
--------------
	*** START SCOPE MAP ***
	Start scope map at 1: T_IF => if
	Process token 2 []: T_WHITESPACE =>  
	Process token 3 []: T_OPEN_PARENTHESIS => (
	* skipping parenthesis *
	Process token 6 []: T_COLON => :
	=> Found scope opener for 1 (T_IF)
	Process token 7 [opener:6;]: T_WHITESPACE =>  
	Process token 8 [opener:6;]: T_CLOSE_TAG => ?>\n
	Process token 9 [opener:6;]: T_INLINE_HTML => 	Something\n
	Process token 10 [opener:6;]: T_OPEN_TAG => <?php 
	Process token 11 [opener:6;]: T_ELSE => else
	* token is an opening condition *
	* searching for opener *
		Process token 12 []: T_COLON => :
		Process token 13 []: T_WHITESPACE =>  
		Process token 14 []: T_CLOSE_TAG => ?>\n
		Process token 15 []: T_INLINE_HTML => 	Something else\n
		Process token 16 []: T_OPEN_TAG => <?php 
		Process token 17 []: T_ENDIF => endif
		Process token 18 []: T_SEMICOLON => ;
		=> Found semicolon before scope opener for 11 (T_ELSE), bailing
	Process token 19 [opener:6;]: T_CLOSE_TAG => ?>\n
	Process token 20 [opener:6;]: T_INLINE_HTML => 	Something that shouldn't be analyzed for the scopes of the if/else\n
	Process token 21 [opener:6;]: T_OPEN_TAG => <?php 
	Process token 22 [opener:6;]: T_IF => if
	* token is an opening condition *
	* searching for opener *
		Process token 23 []: T_WHITESPACE =>  
		Process token 24 []: T_OPEN_PARENTHESIS => (
		* skipping parenthesis *
		Process token 27 []: T_COLON => :
		=> Found scope opener for 22 (T_IF)
		Process token 28 [opener:27;]: T_WHITESPACE =>  
		Process token 29 [opener:27;]: T_CLOSE_TAG => ?>\n
		Process token 30 [opener:27;]: T_INLINE_HTML => 	another thing\n
		Process token 31 [opener:27;]: T_OPEN_TAG => <?php 
		Process token 32 [opener:27;]: T_ELSE => else
		* token is an opening condition *
		* searching for opener *
			Process token 33 []: T_COLON => :
			Process token 34 []: T_WHITESPACE =>  
			Process token 35 []: T_CLOSE_TAG => ?>\n
			Process token 36 []: T_INLINE_HTML => 	another something else\n
			Process token 37 []: T_OPEN_TAG => <?php 
			Process token 38 []: T_ENDIF => endif
			Process token 39 []: T_SEMICOLON => ;
			=> Found semicolon before scope opener for 32 (T_ELSE), bailing
		Process token 40 [opener:27;]: T_WHITESPACE =>  
		Process token 41 [opener:27;]: T_CLOSE_TAG => ?>\n
	Process token 23 [opener:6;]: T_WHITESPACE =>  
	Process token 24 [opener:6;]: T_OPEN_PARENTHESIS => (
	* skipping parenthesis *
	Process token 27 [opener:6;]: T_COLON => :
	Process token 28 [opener:6;]: T_WHITESPACE =>  
	Process token 29 [opener:6;]: T_CLOSE_TAG => ?>\n
	Process token 30 [opener:6;]: T_INLINE_HTML => 	another thing\n
	Process token 31 [opener:6;]: T_OPEN_TAG => <?php 
	Process token 32 [opener:6;]: T_ELSE => else
	* token is an opening condition *
	* searching for opener *
		Process token 33 []: T_COLON => :
		Process token 34 []: T_WHITESPACE =>  
		Process token 35 []: T_CLOSE_TAG => ?>\n
		Process token 36 []: T_INLINE_HTML => 	another something else\n
		Process token 37 []: T_OPEN_TAG => <?php 
		Process token 38 []: T_ENDIF => endif
		Process token 39 []: T_SEMICOLON => ;
		=> Found semicolon before scope opener for 32 (T_ELSE), bailing
	Process token 40 [opener:6;]: T_WHITESPACE =>  
	Process token 41 [opener:6;]: T_CLOSE_TAG => ?>\n
	Start scope map at 11: T_ELSE => else
	Process token 12 []: T_COLON => :
	Process token 13 []: T_WHITESPACE =>  
	Process token 14 []: T_CLOSE_TAG => ?>\n
	Process token 15 []: T_INLINE_HTML => 	Something else\n
	Process token 16 []: T_OPEN_TAG => <?php 
	Process token 17 []: T_ENDIF => endif
	Process token 18 []: T_SEMICOLON => ;
	=> Found semicolon before scope opener for 11 (T_ELSE), bailing
	Start scope map at 22: T_IF => if
	Process token 23 []: T_WHITESPACE =>  
	Process token 24 []: T_OPEN_PARENTHESIS => (
	* skipping parenthesis *
	Process token 27 []: T_COLON => :
	=> Found scope opener for 22 (T_IF)
	Process token 28 [opener:27;]: T_WHITESPACE =>  
	Process token 29 [opener:27;]: T_CLOSE_TAG => ?>\n
	Process token 30 [opener:27;]: T_INLINE_HTML => 	another thing\n
	Process token 31 [opener:27;]: T_OPEN_TAG => <?php 
	Process token 32 [opener:27;]: T_ELSE => else
	* token is an opening condition *
	* searching for opener *
		Process token 33 []: T_COLON => :
		Process token 34 []: T_WHITESPACE =>  
		Process token 35 []: T_CLOSE_TAG => ?>\n
		Process token 36 []: T_INLINE_HTML => 	another something else\n
		Process token 37 []: T_OPEN_TAG => <?php 
		Process token 38 []: T_ENDIF => endif
		Process token 39 []: T_SEMICOLON => ;
		=> Found semicolon before scope opener for 32 (T_ELSE), bailing
	Process token 40 [opener:27;]: T_WHITESPACE =>  
	Process token 41 [opener:27;]: T_CLOSE_TAG => ?>\n
	Start scope map at 32: T_ELSE => else
	Process token 33 []: T_COLON => :
	Process token 34 []: T_WHITESPACE =>  
	Process token 35 []: T_CLOSE_TAG => ?>\n
	Process token 36 []: T_INLINE_HTML => 	another something else\n
	Process token 37 []: T_OPEN_TAG => <?php 
	Process token 38 []: T_ENDIF => endif
	Process token 39 []: T_SEMICOLON => ;
	=> Found semicolon before scope opener for 32 (T_ELSE), bailing
	*** END SCOPE MAP ***
	*** START LEVEL MAP ***
	Process token 0 on line 1 [lvl:0;]: T_OPEN_TAG => <?php 
	Process token 1 on line 1 [lvl:0;]: T_IF => if
	Process token 2 on line 1 [lvl:0;]: T_WHITESPACE =>  
	Process token 3 on line 1 [lvl:0;]: T_OPEN_PARENTHESIS => (
	Process token 4 on line 1 [lvl:0;]: T_VARIABLE => $test
	Process token 5 on line 1 [lvl:0;]: T_CLOSE_PARENTHESIS => )
	Process token 6 on line 1 [lvl:0;]: T_COLON => :
	Process token 7 on line 1 [lvl:0;]: T_WHITESPACE =>  
	Process token 8 on line 1 [lvl:0;]: T_CLOSE_TAG => ?>\n
	Process token 9 on line 2 [lvl:0;]: T_INLINE_HTML => 	Something\n
	Process token 10 on line 3 [lvl:0;]: T_OPEN_TAG => <?php 
	Process token 11 on line 3 [lvl:0;]: T_ELSE => else
	Process token 12 on line 3 [lvl:0;]: T_COLON => :
	Process token 13 on line 3 [lvl:0;]: T_WHITESPACE =>  
	Process token 14 on line 3 [lvl:0;]: T_CLOSE_TAG => ?>\n
	Process token 15 on line 4 [lvl:0;]: T_INLINE_HTML => 	Something else\n
	Process token 16 on line 5 [lvl:0;]: T_OPEN_TAG => <?php 
	Process token 17 on line 5 [lvl:0;]: T_ENDIF => endif
	Process token 18 on line 5 [lvl:0;]: T_SEMICOLON => ;
	Process token 19 on line 5 [lvl:0;]: T_CLOSE_TAG => ?>\n
	Process token 20 on line 6 [lvl:0;]: T_INLINE_HTML => 	Something that shouldn't be analyzed for the scopes of the if/else\n
	Process token 21 on line 7 [lvl:0;]: T_OPEN_TAG => <?php 
	Process token 22 on line 7 [lvl:0;]: T_IF => if
	Process token 23 on line 7 [lvl:0;]: T_WHITESPACE =>  
	Process token 24 on line 7 [lvl:0;]: T_OPEN_PARENTHESIS => (
	Process token 25 on line 7 [lvl:0;]: T_VARIABLE => $anothertest
	Process token 26 on line 7 [lvl:0;]: T_CLOSE_PARENTHESIS => )
	Process token 27 on line 7 [lvl:0;]: T_COLON => :
	Process token 28 on line 7 [lvl:0;]: T_WHITESPACE =>  
	Process token 29 on line 7 [lvl:0;]: T_CLOSE_TAG => ?>\n
	Process token 30 on line 8 [lvl:0;]: T_INLINE_HTML => 	another thing\n
	Process token 31 on line 9 [lvl:0;]: T_OPEN_TAG => <?php 
	Process token 32 on line 9 [lvl:0;]: T_ELSE => else
	Process token 33 on line 9 [lvl:0;]: T_COLON => :
	Process token 34 on line 9 [lvl:0;]: T_WHITESPACE =>  
	Process token 35 on line 9 [lvl:0;]: T_CLOSE_TAG => ?>\n
	Process token 36 on line 10 [lvl:0;]: T_INLINE_HTML => 	another something else\n
	Process token 37 on line 11 [lvl:0;]: T_OPEN_TAG => <?php 
	Process token 38 on line 11 [lvl:0;]: T_ENDIF => endif
	Process token 39 on line 11 [lvl:0;]: T_SEMICOLON => ;
	Process token 40 on line 11 [lvl:0;]: T_WHITESPACE =>  
	Process token 41 on line 11 [lvl:0;]: T_aCLOSE_TAG => ?>\n
	*** END LEVEL MAP ***

Comments

[2013-12-20 06:48 UTC] squiz (Greg Sherwood)

-Status:      Open
+Status:      Verified
-Assigned To:
+Assigned To: squiz
I think you're on the right track, but there are quite a few unit test failures with those 
changes, so it is affecting standard syntax as well. I'll see if I can figure out fix that 
works with both.

[2013-12-24 16:47 UTC] yannicmahe (Yannic Mahe)

Indeed, it breaks quite a lot of tests.

I think that the problem is in the handling of the "shared" and "with" options.

It works properly for cases and default tokens because they have a really special closer that is not shared with any other flow control instruction.

When we use these options for if/else/elesif, the thing is that after finding the closer for an "if", an "else" or an "elseif", we put the stack pointer back just before the closer. As a result, it's found as a closer for other stuff, like a T_FUNCTION for instance in the "CyclomaticComplexityUnitTest.inc" test file.

So we have to find a way to add a check to make sure a closer isn't considered as shared with unproper tokens.
It doesn't look that trivial. I'll keep looking forward for a proper solution as I have some time to spend lately.

[2013-12-24 20:16 UTC] yannicmahe (Yannic Mahe)

I think I found how to fix it without breaking other things.

I was wrong about the way to declare T_IF, T_ELSEIF and T_ELSE. It should be done that way in fact :
T_IF => array(
	'start'  => array(
	                T_OPEN_CURLY_BRACKET,
	                T_COLON,
	            ),
	'end'    => array(
	                T_CLOSE_CURLY_BRACKET,
	                T_ENDIF,
	                T_ELSE,
	                T_ELSEIF,
	            ),
	'strict' => false,
	'shared' => false,
	'with'   => array(),
	),

T_ELSE => array(
        'start'  => array(
	                T_OPEN_CURLY_BRACKET,
	                T_COLON,
	            ),
        'end'    => array(
	                T_CLOSE_CURLY_BRACKET,
	                T_ENDIF,
	            ),
        'strict' => false,
        'shared' => false,
        'with'   => array(),
       ),
T_ELSEIF => array(
        'start'  => array(
	                T_OPEN_CURLY_BRACKET,
	                T_COLON,
	                ),
        'end'    => array(
	                T_CLOSE_CURLY_BRACKET,
	             	   T_ENDIF,
	                T_ELSE,
	                T_ELSEIF,
	            ),
        'strict' => false,
        'shared' => false,
        'with'   => array(),
       ),

Indeed, the "endif" isn't effectively shared. The else and elseif are rather end tokens for previous if and elseif token in fact.

However, to have things work properly, it involves a few changes in File.php.

I was able to run all unit tests without a failure, but I'll check thing a bit further before submiting a patch.

Merry Christmas.

[2013-12-26 15:45 UTC] yannicmahe (Yannic Mahe)

Added patch if-else-elseif-fix.

[2013-12-26 16:12 UTC] yannicmahe (Yannic Mahe)

Added patch if-else-elseif-fix.

[2013-12-26 16:16 UTC] yannicmahe (Yannic Mahe)

It looks like things are working properly. At least it passes all tests successfully on my computer.

I uploaded the patch (actually 2, as I forgot half of it the first time. The first one should be ignored).

I hope I haven't forgotten anything.

[2014-01-06 04:58 UTC] squiz (Greg Sherwood)

Thanks for all your work on this, and sorry I was so quiet over the holidays.

The changes to the tokenizer look good, but the only change I really needed in 
File.php was the one that ensures a curly brace is used to close a block if a curly 
brace was also used to open it. That change makes a lot of sense and gets all the unit 
tests working.

Do you have code that requires the other changes to File.php?

[2014-01-06 05:07 UTC] squiz (Greg Sherwood)

Oh sorry, I had made a couple of other changes to the PHP tokenizer that didn't show 
up the original error that you extra changes were obviously fixing.

The changes I made were
T_ELSE:  'with'   => array(T_IF, T_ELSEIF),
T_ELSEIF: 'with'   => array(T_IF, T_ELSE),

I'm interested to know if those 2 changes work for your code.

[2014-01-06 05:09 UTC] squiz (Greg Sherwood)

Probably easier to just look here: https://gist.github.com/gsherwood/8275236

[2014-01-06 23:02 UTC] yannicmahe (Yannic Mahe)

In fact, I don't think that the scopes are properly set that way.

You'll find below the script I use for my testings :

<?php if ($test): ?>
    Something
<?php else: ?>
    Something else
<?php endif;?>
Something that shouldn't be analyzed for the scopes of the if/else
<?php if ($anothertest): ?>
    another thing
<?php elseif ($stillanothertest): ?>
    still another thing
<?php else: ?>
    another something else
<?php endif; ?> 


To see what's wrong I changed File.php to display the types of closer tokens found (line 1668 in 1.5.1 release file) :

if (PHP_CODESNIFFER_VERBOSITY > 1) {
    $type = $tokens[$stackPtr]['type'];
    echo str_repeat("\t", $depth);
    echo "=> Found scope closer for $stackPtr ($type) : token $i (" . $tokens[$i]['type'] . ")" .PHP_EOL;
}


Here is the result with your code :

*** START SCOPE MAP ***
Start scope map at 1: T_IF => if
Process token 2 []: T_WHITESPACE =>  
Process token 3 []: T_OPEN_PARENTHESIS => (
* skipping parenthesis *
Process token 6 []: T_COLON => :
=> Found scope opener for 1 (T_IF)
Process token 7 [opener:6;]: T_WHITESPACE =>  
Process token 8 [opener:6;]: T_CLOSE_TAG => ?>\n
Process token 9 [opener:6;]: T_INLINE_HTML =>     Something\n
Process token 10 [opener:6;]: T_OPEN_TAG => <?php 
Process token 11 [opener:6;]: T_ELSE => else
* token is an opening condition *
* searching for opener *
	Process token 12 []: T_COLON => :
	=> Found scope opener for 11 (T_ELSE)
	Process token 13 [opener:12;]: T_WHITESPACE =>  
	Process token 14 [opener:12;]: T_CLOSE_TAG => ?>\n
	Process token 15 [opener:12;]: T_INLINE_HTML =>     Something else\n
	Process token 16 [opener:12;]: T_OPEN_TAG => <?php 
	Process token 17 [opener:12;]: T_ENDIF => endif
	=> Found scope closer for 11 (T_ELSE) : token 17 (T_ENDIF)
=> Found scope closer for 1 (T_IF) : token 17 (T_ENDIF)
Start scope map at 22: T_IF => if
Process token 23 []: T_WHITESPACE =>  
Process token 24 []: T_OPEN_PARENTHESIS => (
* skipping parenthesis *
Process token 27 []: T_COLON => :
=> Found scope opener for 22 (T_IF)
Process token 28 [opener:27;]: T_WHITESPACE =>  
Process token 29 [opener:27;]: T_CLOSE_TAG => ?>\n
Process token 30 [opener:27;]: T_INLINE_HTML =>     another thing\n
Process token 31 [opener:27;]: T_OPEN_TAG => <?php 
Process token 32 [opener:27;]: T_ELSEIF => elseif
* token is an opening condition *
* searching for opener *
	Process token 33 []: T_WHITESPACE =>  
	Process token 34 []: T_OPEN_PARENTHESIS => (
	* skipping parenthesis *
	Process token 37 []: T_COLON => :
	=> Found scope opener for 32 (T_ELSEIF)
	Process token 38 [opener:37;]: T_WHITESPACE =>  
	Process token 39 [opener:37;]: T_CLOSE_TAG => ?>\n
	Process token 40 [opener:37;]: T_INLINE_HTML =>     still another thing\n
	Process token 41 [opener:37;]: T_OPEN_TAG => <?php 
	Process token 42 [opener:37;]: T_ELSE => else
	* token is an opening condition *
	* searching for opener *
		Process token 43 []: T_COLON => :
		=> Found scope opener for 42 (T_ELSE)
		Process token 44 [opener:43;]: T_WHITESPACE =>  
		Process token 45 [opener:43;]: T_CLOSE_TAG => ?>\n
		Process token 46 [opener:43;]: T_INLINE_HTML =>     another something else\n
		Process token 47 [opener:43;]: T_OPEN_TAG => <?php 
		Process token 48 [opener:43;]: T_ENDIF => endif
		=> Found scope closer for 42 (T_ELSE) : token 48 (T_ENDIF)
	=> Found scope closer for 32 (T_ELSEIF) : token 48 (T_ENDIF)
=> Found scope closer for 22 (T_IF) : token 48 (T_ENDIF)
*** END SCOPE MAP ***


And here is the result of the code I proposed :

*** START SCOPE MAP ***
Start scope map at 1: T_IF => if
Process token 2 []: T_WHITESPACE =>  
Process token 3 []: T_OPEN_PARENTHESIS => (
* skipping parenthesis *
Process token 6 []: T_COLON => :
=> Found scope opener for 1 (T_IF)
Process token 7 [opener:6;]: T_WHITESPACE =>  
Process token 8 [opener:6;]: T_CLOSE_TAG => ?>\n
Process token 9 [opener:6;]: T_INLINE_HTML =>     Something\n
Process token 10 [opener:6;]: T_OPEN_TAG => <?php 
Process token 11 [opener:6;]: T_ELSE => else
* token is an opening condition *
* searching for opener *
	Process token 12 []: T_COLON => :
	=> Found scope opener for 11 (T_ELSE)
	Process token 13 [opener:12;]: T_WHITESPACE =>  
	Process token 14 [opener:12;]: T_CLOSE_TAG => ?>\n
	Process token 15 [opener:12;]: T_INLINE_HTML =>     Something else\n
	Process token 16 [opener:12;]: T_OPEN_TAG => <?php 
	Process token 17 [opener:12;]: T_ENDIF => endif
	=> Found scope closer for 11 (T_ELSE) : token 17 (T_ENDIF)
=> Found scope closer for 1 (T_IF) : token 11 (T_ELSE)
Start scope map at 22: T_IF => if
Process token 23 []: T_WHITESPACE =>  
Process token 24 []: T_OPEN_PARENTHESIS => (
* skipping parenthesis *
Process token 27 []: T_COLON => :
=> Found scope opener for 22 (T_IF)
Process token 28 [opener:27;]: T_WHITESPACE =>  
Process token 29 [opener:27;]: T_CLOSE_TAG => ?>\n
Process token 30 [opener:27;]: T_INLINE_HTML =>     another thing\n
Process token 31 [opener:27;]: T_OPEN_TAG => <?php 
Process token 32 [opener:27;]: T_ELSEIF => elseif
* token is an opening condition *
* searching for opener *
	Process token 33 []: T_WHITESPACE =>  
	Process token 34 []: T_OPEN_PARENTHESIS => (
	* skipping parenthesis *
	Process token 37 []: T_COLON => :
	=> Found scope opener for 32 (T_ELSEIF)
	Process token 38 [opener:37;]: T_WHITESPACE =>  
	Process token 39 [opener:37;]: T_CLOSE_TAG => ?>\n
	Process token 40 [opener:37;]: T_INLINE_HTML =>     still another thing\n
	Process token 41 [opener:37;]: T_OPEN_TAG => <?php 
	Process token 42 [opener:37;]: T_ELSE => else
	* token is an opening condition *
	* searching for opener *
		Process token 43 []: T_COLON => :
		=> Found scope opener for 42 (T_ELSE)
		Process token 44 [opener:43;]: T_WHITESPACE =>  
		Process token 45 [opener:43;]: T_CLOSE_TAG => ?>\n
		Process token 46 [opener:43;]: T_INLINE_HTML =>     another something else\n
		Process token 47 [opener:43;]: T_OPEN_TAG => <?php 
		Process token 48 [opener:43;]: T_ENDIF => endif
		=> Found scope closer for 42 (T_ELSE) : token 48 (T_ENDIF)
	=> Found scope closer for 32 (T_ELSEIF) : token 42 (T_ELSE)
=> Found scope closer for 22 (T_IF) : token 32 (T_ELSEIF)
*** END SCOPE MAP ***

You will note the scope closers found with both codes :
1. yours is finding T_ENDIF
2. mine is finding T_ELSE or T_ELSEIF

I don't think you should consider the T_ENDIF as the closer for all the tokens as it creates scopes overlaps (T_IF scope includes the T_ELSE scope).
It doesn't look consistant with the standard if/else syntax.

I get what's going on, but it's really hard to clearly explain it (I could hardly in my mother tongue, so in English...).
To try to be short, here are the main problems :
1. the condition at line 1655, in File.php of release 1.5.1, can now be met after a recursive call (it couldn't before).
Indeed, a T_ELSE (or a T_ELSEIF) can be an "opening condition" and a closer token for T_IF at the same time
2. while in this condition, there is an inconsistency between the values of $i and $tokenType :
- $i refers to a closer token (T_ENDIF)
- $tokenType refers to an opening condition (T_ELSE or T_ELSEIF)

The $newIndex thing is to fix that inconsistency which leads to a wrong token selected as the closer for the T_IF.
That way, we only advance to the next unhandled token after all processing on the current token has been done (which is check whether a T_ELSE or a T_ELSEIF token is a closer for a T_IF).

I just hope you'll get what I tried to explain.

[2014-01-09 06:03 UTC] squiz (Greg Sherwood)

Thanks, I understand your problem. I've updated the gist with another possible fix: 
https://gist.github.com/gsherwood/8275236

The biggest change is that PHPCS now allows a single token to both be a scope closer 
and a scope opener at the same time, as long as the "shared with" token values are 
correct.

[2014-01-10 22:19 UTC] yannicmahe (Yannic Mahe)

Hi, and thanks for your fast reply.

Currently I can't access gist.github.com because of business proxy rules. I have issued a request to be able to view your patch, but it shouldn't be handled before next week.
Proxy policy is quite strict in my company.

So I'll have a look a this on next week.

[2014-01-15 19:02 UTC] yannicmahe (Yannic Mahe)

Hi.

After doing tests, everything looks fine with your last patch.

You can close this bug.

Thank you.

[2014-01-22 09:40 UTC] squiz (Greg Sherwood)

-Status: Verified
+Status: Closed
Thanks a lot for helping to figure this one out. Commit is here: 
https://github.com/squizlabs/PHP_CodeSniffer/commit/5f445dfea5f5a8677513a34f228
0be4dc583253d