Weird behavior of ControlStructures::getDeclareScopeOpenClose method
Closed this issue · 9 comments
When writing a sniff for detecting and allowing or disallowing brace usage in the declare
construct I used the ControlStructures::getDeclareScopeOpenClose
helper to detect if declare
is using the braces or not.
One of the cases I'm trying to block out early is the usage of strict_types
directive with curly braces, as this is forbidden and will throw a fatal error.
One code that I've tested is this one
<?php
declare(strict_types=1) // Error.
echo 'hi!';
{
// Code.
}
At first, I thought I'd get a parse error if I put anything before the curly brace and the declare
keyword, but I got the Fatal error: strict_types declaration must not use block mode
instead.
So I tried seeing if the same would be correct for encoding
directive (as this one allows curly brace usage). I had to test this one locally because it requires the usage of the Zend multibyte extension which is off in the 3v4l.
By default
<?php
echo 'aäaß' . "\n";
when ran will just echo out these characters. Then I change the encoding
<?php
declare(encoding='ISO-8859-15');
echo 'aäaß' . "\n";
And I get aÀaÃ
out. Then I wrap it in the braces to confirm it's working:
<?php
declare(encoding='ISO-8859-15') {
echo 'aäaß' . "\n";
}
And the result is like the previous one. Then I add an echo statement between the declare and the opening brace
<?php
declare(encoding='ISO-8859-15')
echo 'aäaß' . "\n";
{
echo 'aäaß' . "\n";
}
And (to my surprise) I get:
aÀaÃ
aÀaÃ
Then I add this
<?php
declare(encoding='ISO-8859-15')
echo 'aäaß' . "\n";
{
echo 'aäaß' . "\n";
}
echo 'aäaß' . "\n";
And I get
aÀaÃ
aÀaÃ
aÀaÃ
So it turns out that, no matter how this construct is called it's going to be applied to the entire file. Regardless of the scope.
I'm not sure if this is by design or a bug.
But the issue is that when you have anything else that is not a comment between the declare statement and the opening brace, the getDeclareScopeOpenClose
will then return false
, and it will behave like there is no curly braces at all.
This in turn makes it so that the original code in question:
<?php
declare(strict_types=1) // Error.
echo 'hi!';
{
// Code.
}
Will return false for getDeclareScopeOpenClose
, when it should be caught (as it will error out).
Again, I'm not sure if this is intentional, or if I should just ignore the code like that when checking for the scope opener and closer for the declare
construct 🤷🏼♂️
To be honest, this sounds like a bug which should be reported to PHP Core.
The ControlStructures::getDeclareScopeOpenClose()
function is based on the way PHP has documented that the block mode should be used with declare()
directives.
Also note the "Caution" in the docs (which IIRC the helper doesn't check for as it should allow for sniffs to detect this error).
If PHP doesn't throw a parse error for those code samples you showed above, to me, that would mean that either the docs needs to be updated or a bug needs to be fixed in PHP.
I'm not sure how the ControlStructures::getDeclareScopeOpenClose()
function should behave in such cases. I think we first needs clarification of whether the behaviour of PHP in this case is intentional or a bug.
P.S.: nice find!
Note: in the above code samples, I'm missing one, which I'd be interested to see the result of:
<?php
declare(encoding='ISO-8859-15') {
echo 'aäaß' . "\n";
}
echo 'aäaß' . "\n";
☝🏻 This should help determine whether PHP respects the braces, but only if directly after the declare
or if the braces have no effect.
The syntax of declare is similar to the syntax of other flow control constructs:
declare (directive) statement
Based on this bit in the manual, I have a niggly feeling that a declare
statement without a semi-colon and without a curly open brace straight after the close parenthesis, is supposed to only apply to the first statement directly following it (like control structures without braces), but based on your tests, if that is the intentional behaviour, it is not working correctly if the declare then still affects statements after the first statement.
Oh... this is also a lovely comment... https://www.php.net/manual/en/control-structures.declare.php#124553
It's possible to set directives at one time if every directive is supported.
<?php declare(strict_types=1, encoding='UTF-8');
Never come across that before. guess I better fix the PHPCS upstream sniff for that as well....
<?php declare(encoding='ISO-8859-15') { echo 'aäaß' . "\n"; } echo 'aäaß' . "\n";
This one also returns the
aÀaÃ
aÀaÃ
I'll open a bug report at php-src to see what the response will be.
@dingo-d As this method is redundant now PHPCS 3.7.1 is the minimum supported PHPCS version, it has been deprecated in #347 (in favour of checking the scope_opener
/scope_closer
indexes directly on the T_DECLARE
) and will be removed before the final 1.0.0
release.
With that in mind, can this issue be closed, as I don't think there is anything actionable for PHPCSUtils ?
The only thing from the above thread which I think might be interesting for PHPCSUtils could be to offer a function to parse declare
statements into the individual directives, returning an array of arrays with directive
, directive_token
, value
, value_token
(or something along those lines).
This could help standards which examine declare statements to handle multi-directive statements correctly.
Then again, it may be better/more straight-forward to add T_DECLARE
to the list of tokens parsable by the PassedParameters
class (and ensure that it is supported correctly) ?
What do you think ? If you think this would be a good idea, we should probably open a separate issue for that.
We can close this issue.
As for the parsing the declare statements, that could be useful, although currently i'm just checking string toke s inside the directive openers and closers, so it's not that extra of an overhead that would require a new utility method for 🤷♂️
I think reusing PssedParameters
might be a better solution 👍