Tests/Tokenizer: fix bug in the default keyword tests

[rodrigoprimo]

Description

This PR fixes a bug in RecurseScopeMapDefaultKeywordConditionsTest ::testSwitchDefault(). Among other things, this method checks whether tokens within the scope of a default have T_DEFAULT set as the scope condition.

But the code would never check if the scope condition is set for the tokens within the scope of a T_DEFAULT token when $conditionStop is not null. $conditionStop is used when T_DEFAULT uses curlies to open and close the scope. In those cases, scope conditions are not set all the way until the scope closer. Instead, they are set only until just before a T_BREAK|T_RETURN|T_CONTINUE.

simple_switch_default_with_curlies test case is the only one that currently sets $conditionStop to something other than null. But it passes an offset value of the condition stop while the code was expecting an absolute value of the index of the token where the scope condition stops being set. Passing an offset meant that when $end was set to be equal to $conditionStop it would always be less than $i and the assertion inside the for loop would never run:

PHP_CodeSniffer/tests/Core/Tokenizers/Tokenizer/RecurseScopeMapDefaultKeywordConditionsTest.php

Lines 160 to 170 in be74da1

	if (isset($conditionStop) === true) {
	$end = $conditionStop;
	}
	for ($i = ($opener + 1); $i < $end; $i++) {
	$this->assertArrayHasKey(
	$token,
	$tokens[$i]['conditions'],
	'T_DEFAULT condition not added for token belonging to the T_DEFAULT structure'
	);
	}

Now, the code calculates the correct value for $end when $conditionStop is not null. It was also necessary to update the value for $conditionStop in simple_switch_default_with_curlies as it was pointing to the wrong token.

This uncovered an inconsistency in the tokenizer that might need to be addressed in a separate issue. When T_DEFAULT doesn't use curlies the tokenizer sets scope_condition for all the tokens until the token just before T_BREAK|T_RETURN|T_CONTINUE. But when there are curlies, scope_condition is set including for T_BREAK|T_RETURN|T_CONTINUE. It needs to be determined if this behavior is intentional or not. Let me know if I should create an issue for us to investigate this.

Related issues/external references

This problem was found while working on #813.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
  • This change is only breaking for integrators, not for external standards or end-users.
• Documentation improvement

PR checklist

• I have checked there is no other PR open for the same change.
• I have read the Contribution Guidelines.
• I grant the project the right to include and distribute the code under the BSD-3-Clause license (and I have the right to grant these rights).
• I have added tests to cover my changes.
• I have verified that the code complies with the projects coding standards.
• [Required for new sniffs] I have added XML documentation for the sniff.

[rodrigoprimo]

In those cases, scope conditions are not set all the way until the scope closer. Instead, they are set only until just before a T_BREAK|T_RETURN|T_CONTINUE.

I'm basing the above on this code comment:

PHP_CodeSniffer/tests/Core/Tokenizers/Tokenizer/RecurseScopeMapDefaultKeywordConditionsTest.php

Lines 192 to 195 in 2ab8974

	// For a default structure with curly braces, the scope opener
	// will be the open curly and the closer the close curly.
	// However, scope conditions will not be set for open to close,
	// but only for the open token up to the "break/return/continue" etc.

[jrfnl]

This uncovered an inconsistency in the tokenizer that might need to be addressed in a separate issue. When T_DEFAULT doesn't use curlies the tokenizer sets scope_condition for all the tokens until the token just before T_BREAK|T_RETURN|T_CONTINUE. But when there are curlies, scope_condition is set including for T_BREAK|T_RETURN|T_CONTINUE. It needs to be determined if this behavior is intentional or not. Let me know if I should create an issue for us to investigate this.

Until I look into this I don't know whether this needs fixing or not (and whether it is even possible to fix), so yes, please open an issue and use the "Status: needs investigation" label. Do not mark this as a "Bug" as it is not yet determined whether it actually is one or not.

Edit - this may be related to/a duplicate of: squizlabs/PHP_CodeSniffer#3794

[rodrigoprimo]

Thanks for your review, @jrfnl. I implemented the changes that you suggested. Could you please take another look at this PR?

Until I look into this I don't know whether this needs fixing or not (and whether it is even possible to fix), so yes, please open an issue and use the "Status: needs investigation" label. Do not mark this as a "Bug" as it is not yet determined whether it actually is one or not.

Here is the issue: #862

While creating it, I noticed that I made a mistake when describing the issue in this PR. Where I wrote scope_condition, I should have written conditions.