Weird behavior of ControlStructures::getDeclareScopeOpenClose method

[dingo-d]

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 🤷🏼‍♂️

[jrfnl]

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!

[jrfnl]

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";

[jrfnl]

☝🏻 This should help determine whether PHP respects the braces, but only if directly after the declare or if the braces have no effect.

[jrfnl]

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.

[jrfnl]

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....

[dingo-d]

<?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.

[jrfnl]

@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.

[dingo-d]

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 👍

[jrfnl]

See: #378