Skip to content

Commit c9ca9b3

Browse files
committed
Avoid PHP warnings for break & continue
Our previous mechanism of doing a "lookbehind" with twig lead to array key warnings. Instead we now look ahead, which is also valid from the perspective of twig, to determine if more loops are ending than closing, and for break if enough loops are ending to break out of.
1 parent 5f3172d commit c9ca9b3

File tree

2 files changed

+14
-17
lines changed

2 files changed

+14
-17
lines changed

composer.json

+1-1
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@
4747
"psalm_base": "vendor/bin/psalm --set-baseline=psalm-baseline.xml",
4848
"phpunit": "vendor/bin/phpunit --colors=always",
4949
"phpunit_clover": "vendor/bin/phpunit --coverage-text --coverage-clover build/logs/clover.xml",
50-
"coverage": "vendor/bin/phpunit --coverage-html tests/_reports",
50+
"coverage": "XDEBUG_MODE=coverage vendor/bin/phpunit --coverage-html tests/_reports",
5151
"phpcs": "vendor/bin/phpcs --standard=ruleset.xml --extensions=php --cache=.phpcs-cache --colors src tests",
5252
"phpcsfix": "vendor/bin/phpcbf --standard=ruleset.xml --extensions=php --cache=.phpcs-cache src tests",
5353
"binupdate": "@composer bin all update --ansi",

src/TokenParser/BreakOrContinueTokenParser.php

+13-16
Original file line numberDiff line numberDiff line change
@@ -33,40 +33,37 @@ public function parse(Token $token): Node
3333

3434
$stream->expect(Token::BLOCK_END_TYPE);
3535

36-
// Trick to check if we are currently in a loop.
37-
$currentForLoop = 0;
36+
// Count how many loops are starting minus the loops ending
37+
$loopCount = 0;
3838

3939
for ($i = 1; true; $i++) {
40-
// If we look back too far in the stream twig will throw a SyntaxError
41-
// and there we often get another error because an illegal offset is accessed
42-
// when creating the SyntaxError, so we catch all throwables to be safe
4340
try {
44-
// We look back by using a negative integer, which is not intended by twig and seems
45-
// a bit hacky, but does work for our purposes currently and gives us the opportunity
46-
// to detect illegal usages of break/continue outside of loops or in not-enough-nested contexts
47-
$token = $stream->look(-$i);
48-
} catch (\Throwable $e) {
41+
// Look ahead to find for and endfor tokens to make sure
42+
// there are more loops ending than starting
43+
$token = $stream->look($i);
44+
} catch (SyntaxError $e) {
45+
// End of template, leading to SyntaxError
4946
break;
5047
}
5148

5249
// Count both "for" loops and "foreach" loops
5350
if ($token->test(Token::NAME_TYPE, 'for') || $token->test(Token::NAME_TYPE, 'foreach')) {
54-
$currentForLoop++;
51+
$loopCount++;
5552
} elseif ($token->test(Token::NAME_TYPE, 'endfor') || $token->test(Token::NAME_TYPE, 'endforeach')) {
56-
$currentForLoop--;
53+
$loopCount--;
5754
}
5855
}
5956

60-
61-
if ($currentForLoop < 1) {
57+
// There should be more loops ending than starting, making loopCount negative
58+
if ($loopCount >= 0) {
6259
throw new SyntaxError(
6360
\ucfirst($this->getTag()) . ' tag is only allowed in \'for\' or \'foreach\' loops.',
6461
$stream->getCurrent()->getLine(),
6562
$stream->getSourceContext()
6663
);
67-
} elseif ($currentForLoop < $loopNumber) {
64+
} elseif (\abs($loopCount) < $loopNumber) {
6865
throw new SyntaxError(
69-
\ucfirst($this->getTag()) . ' tag uses a loop number higher than the actual loops in this context - you are using the number ' . $loopNumber . ' but in the given context the maximum number is ' . $currentForLoop . '.',
66+
\ucfirst($this->getTag()) . ' tag uses a loop number higher than the actual loops in this context - you are using the number ' . $loopNumber . ' but in the given context the maximum number is ' . \abs($loopCount) . '.',
7067
$stream->getCurrent()->getLine(),
7168
$stream->getSourceContext()
7269
);

0 commit comments

Comments
 (0)