Added new sniff Generic.PHP.DeclareStrictTypes #1771

michalbundyra · 2017-12-06T13:45:05Z

Configuration option:

~~omitCommentWithTags - tags names, comment with one of these tags will be omitted and declaration will be added below the comment (default: ['@author', '@copyright', '@license'],~~
format - how declaration should look like (default declare(strict_types=1);).
~~linesBefore~~ spacesBefore - number of blank lines before the declaration (default 1)
~~linesAfter~~ spacesAfter - number of lines after the declaration and before the script (default 1)

Resolves #911

/cc @Xerkus

src/Standards/Generic/Sniffs/PHP/DeclareStrictTypesSniff.php

jrfnl · 2017-12-31T10:21:53Z

@webimpress 👍

Just wondering whether it would be better to have two sniffs:

one to require a declare strict statement
one to check the formatting

The reason would be that some standards may require a strict statement, but haven't got a strict guideline on how it should be formatted.
Or similarly, a standard which doesn't require strict typing for every file, but does want the statement to be formatted in a certain way if it exists.

Of course, the other errorcodes could be silenced or - if using PHPCS 3.x - the NotFound errorcode could be included on its own, but having two separate sniffs might be simpler and more discoverable ?

jrfnl

I've just had a look in detail at the code of the sniff and have found a couple of additional situations which you may want to account for.

Hope it helps!

Example 1

<?php
declare(ticks=1) {}

declare(strict_types=1);

will generate the following report:

----------------------------------------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 1 LINE
----------------------------------------------------------------------------------------------------
 2 | ERROR | [x] Invalid number of blank lines before declare statement; expected 1, but found 0
   |       |     (Generic.PHP.DeclareStrictTypes.LinesBefore)
 2 | ERROR | [x] Invalid format of declaration; expected "declare(strict_types=1);", but found
   |       |     "declare(ticks=1) {}
   |       |
   |       |     declare(strict_types=1);"
   |       |     (Generic.PHP.DeclareStrictTypes.InvalidFormat)
----------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------------------------------------

I would expect a NotFirst kind of error instead, i.e. "Strict type declaration has to be the first statement in a file".
Examining the ticks declaration as if it is the strict declaration feels wrong.

More painful, the auto-fixer fixes this to:

<?php

declare(strict_types=1);

In other words: the whole ticks statement is removed by the fixer.

Example 2

<html>
    <head>
        <title>
            <?php
            declare(strict_types=1);
            echo 'Title';
            ?>
        </title>
    </head>

    <?php echo $content; ?>
</html>

will generate the following report:

----------------------------------------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 1 LINE
----------------------------------------------------------------------------------------------------
 5 | ERROR | [x] Invalid number of blank lines before declare statement; expected 1, but found 0
   |       |     (Generic.PHP.DeclareStrictTypes.LinesBefore)
 5 | ERROR | [x] Invalid number of blank lines after declare statement; expected 1, but found 0
   |       |     (Generic.PHP.DeclareStrictTypes.LinesAfter)
----------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------------------------------------

I would an expect an error for the declare statement not being the first content of the file and the auto-fixer to move the statment to above the HTML or for this error not to be auto-fixable.

Example 3

<?php /* Must be first. */ declare(strict_types=1); /* Comment after. */ ?>

This is not seen as a one-line statement and would be auto-fixed to:

<?php /* Must be first. */

declare(strict_types=1);

 /* Comment after. */ ?>

Is this what you intended ?

Example 4

<?php
// Comment
declare(strict_types=1); ?>
<html>
    <?php echo $content; ?>
</html>

will generate the following report:

----------------------------------------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 1 LINE
----------------------------------------------------------------------------------------------------
 3 | ERROR | [x] Invalid number of blank lines before declare statement; expected 1, but found 0
   |       |     (Generic.PHP.DeclareStrictTypes.LinesBefore)
 3 | ERROR | [x] Invalid number of blank lines after declare statement; expected 0, but found -1
   |       |     (Generic.PHP.DeclareStrictTypes.LinesAfter)
----------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------------------------------------

The -1 in the Generic.PHP.DeclareStrictTypes.LinesAfter error message looks incorrect.

This also results in the auto-fixer not adding enough blank lines:

<?php
// Comment

declare(strict_types=1);
 ?>
<html>
    <?php echo $content; ?>
</html>

Take note of the additional whitespace in front of the PHP closing tag in the fixed code.

Example 5

<?php

declare(strict_types=1) /* comment */;

will generate the following report (as expected):

----------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------------------------------------
 3 | ERROR | [x] Invalid format of declaration; expected "declare(strict_types=1);", but found
   |       |     "declare(strict_types=1) /* comment */;"
   |       |     (Generic.PHP.DeclareStrictTypes.InvalidFormat)
----------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------------------------------------

The fixer will however remove the comment, resulting in:

<?php

declare(strict_types=1);

IMHO, comments should never be removed by the auto-fixer. Maybe moved to after the declaration, but not removed. If this is difficult to account for, the error should in that case maybe not be auto-fixable.

jrfnl · 2017-12-31T10:52:03Z

src/Standards/Generic/Sniffs/PHP/DeclareStrictTypesSniff.php

+                    && $tokens[$after]['code'] === T_CLOSE_TAG
+                ) {
+                    if ($tokens[$prev]['line'] !== $tokens[$next]['line']) {
+                        $error = 'PHP open tag must be in the same line as declaration.';


in => on
as declaration => either as strict type declaration or as the declaration

jrfnl · 2017-12-31T10:53:28Z

src/Standards/Generic/Sniffs/PHP/DeclareStrictTypesSniff.php

+                    }
+
+                    if ($tokens[$after]['line'] !== $tokens[$eos]['line']) {
+                        $error = 'PHP close tag must be in the same line as declaration.';


Same spelling/grammar remarks as above for the open tag message.

jrfnl · 2017-12-31T10:59:56Z

src/Standards/Generic/Sniffs/PHP/DeclareStrictTypesSniff.php

+                    $after = false;
+                }//end if
+
+                // Check how many blank lines is before declare statement.


is => there are

Xerkus · 2017-12-31T12:24:43Z

I could have sworn strict types declaration was required to be the first statement in the file. Apparently other declare()s is an exception

michalbundyra · 2018-02-02T14:49:26Z

@jrfnl Thanks for your great feedback. I've addressed almost all of your suggestion in my last update.
I had to remove one test with different configuration option, because now I check also if declare statement is first statement in the file. I don't know how else we can test different configuration options?

I need to check on more case:

<?php
// comment

declare(strict_types=1);

I think in that case, also declaration should be moved before the comment.

Could you have a look please on my changes and let me know what do you think? Thanks!

gsherwood · 2018-03-23T00:20:40Z

Sorry, I've just got to this on my todo list.

I actually don't like the idea of either the omitCommentWithTags or format properties. The omitCommentWithTags mixes this sniff up with documentation sniffs, and the format property assumes everyone is going to want to format their declare line in a very different way. I don't think I'd merge this in with either of those options, so I'll need to know how important they really are.

If I was writing a strict types sniff, I'd write one to enforce the format outlined in PSR-12 because I imagine that will be the way developers will write these statements when they don't have any strong opinion about how to format them or where to place them.

So to be merged in, I think this sniff should only be checking the format of the declare statement, to ensure it looks exactly like declare(strict_types=1); and has a blank line before and after, depending on if there is any content there. The properties would probably be spacingBefore spacingAfter for the number of blank lines (consistent with props used by other sniffs) and maybe even requiredSpacesAfterOpen and requiredSpacesBeforeClose if we think people will want spaces inside the parenthesis.

PSR-12 should be able to make use of this sniff, and then ensure other rules (like the fact this must come after a file comment) are followed using new sniffs.

I'm going to remove this from the 3.3.0 roadmap, but will re-add if we can agree on a new version of the sniff before that release.

- linesBefore -> spacingBefore - linesAfter -> spacingAfter

michalbundyra · 2018-03-26T10:45:49Z

@gsherwood I've updated this PR:

rebased and resolved conflicts;
removed omitCommentWithTags property;
renamed linesBefore and linesAfter properties to spacesBefore and spacesAfter accordingly;

I didn't add requiredSpacesAfterOpen and requiredSpacesBeforeClose. I can't see reason why someone would like to have space after open and no space before close. I think format gives us more flexibility and we know what we should insert by default (if the strict declaration is missing).
We can make this property protected/private if you don't want this flexibility.

Is there any chance to bring it back to 3.3.0 release?

jrfnl · 2018-03-27T13:08:15Z

src/Standards/Generic/Sniffs/PHP/DeclareStrictTypesSniff.php

@@ -310,7 +310,7 @@ public function process(File $phpcsFile, $stackPtr)

                $this->checkOtherDeclarations($phpcsFile, $next);

-                return (count($tokens) + 1);
+                return $phpcsFile->numTokens;


If I remember correctly, elsewhere return ($phpcsFile->numTokens + 1) is used. Shouldn't really make a difference, but just thought I'd mention it for consistency sake.

@jrfnl

- per @jrfnl

michalbundyra · 2018-04-04T06:38:37Z

@gsherwood any chance to re-add it into 3.3.0 release?

ademarco · 2018-04-13T12:20:20Z

@webimpress how challenging would it be to back-port this to 2.9?

WarriorXK · 2018-08-30T14:06:28Z

Is there any progress on this? I would like to include this in our ruleset

player259 · 2018-08-30T15:39:16Z

@WarriorXK You can use SlevomatCodingStandard.TypeHints.DeclareStrictTypes from https://github.com/slevomat/coding-standard. There are many useful sniffs.

floverdevel · 2018-11-15T01:05:27Z

How can i try the Generic.PHP.DeclareStrictTypes ?
Usually i run phpcs with the --exclude=Generic.Files.LineLength --standard=PSR2 options.
Is there a way to run it so i can test this PR?
Maybe something like this :
vendor/bin/phpcs -p --extensions=php --exclude=Generic.Files.LineLength --sniffs=Generic.PHP.DeclareStrictTypes --standard=PSR2 src/ tests/

gsherwood · 2018-11-15T01:35:26Z

@floverdevel To test, you'll need to merge this PR, then create a ruleset.xml file as you can't do what you are after on the command line alone. Make the contents of the file:

<?xml version="1.0"?>
    <ruleset name="MyStandard"> 
    <rule ref="PSR2">
        <exclude name="Generic.Files.LineLength"/>
    </rule>
    <rule ref="Generic.PHP.DeclareStrictTypes"/>
</ruleset>

Then run PHPCS like this: vendor/bin/phpcs -p --extensions=php --standard=/path/to/ruleset.xml src/ tests/

floverdevel · 2018-11-15T01:43:56Z

@gsherwood thank you very much !!!

gmponos

Would you like to add a test case where the declare is in this format: declare( strict_types=1 );

gsherwood · 2018-12-12T03:16:24Z

This sniff still has a public sniff property called format, but that's not how the other sniffs set their rules. Maybe we need to list out how the format of the declare statement could change and how this sniff wants to allow it to change.

Maybe we need a setting for how many spaces there are inside the parenthesis. Maybe another for spaces around the equals sign? Or maybe we release it to enforce the most common format first and add settings later if they are requested.

Edit: did not mean to close - pressed wrong button

floverdevel · 2019-01-08T01:19:38Z

@gsherwood just a quick question : what prevents us from merging this pull-request?
I understand that the code may not be perfect, but it could help many people to use this sniff ... can we merge the code and refactore thereafter?

gsherwood · 2019-01-08T02:13:08Z

can we merge the code and refactore thereafter?

The changes relate to public sniff properties. Changes to these would be BC breaks that can only be made in a major version, so this needs to be right before it is released.

@gsherwood

We don't want this configuration option to be available - per @gsherwood

floverdevel · 2019-02-18T20:00:28Z

Any news on this PR ?
I see that one job failed ...
https://travis-ci.org/squizlabs/PHP_CodeSniffer/jobs/483778385

gsherwood · 2019-04-03T04:07:31Z

Any news on this PR ?

No changed has been made to remove the format property and instead provide sniff properties to configure the format, so the PR won't be merged. I'd even be happy with sniff enforcing a single format only.

I also don't think this sniff should be inserting the declare statement if it doesn't exit. The PHPCS fixers don't modify the behaviour of code - only the formatting of the code.

michalbundyra · 2019-04-06T07:41:23Z

@gsherwood Changes has been made in this PR very long time ago. Property format is private so it is not possible to configure it.

Anyway, I am closing this PR as I can't see any interest of including it in that library.

Everyone who is interested in using sniff like that can use my library:
https://github.com/webimpress/coding-standard
and sniff: WebimpressCodingStandard.Sniffs.DeclareStrictTypes

Dr10s · 2022-07-19T14:19:45Z

You no need configure format property.
You can configure declaration by other public properties:

<rule ref="vendor/slevomat/coding-standard/SlevomatCodingStandard/Sniffs/TypeHints/DeclareStrictTypesSniff.php" >
        <properties>
            <property name="spacesCountAroundEqualsSign" value="0"/>
        </properties>
</rule>

gmponos reviewed Dec 6, 2017

View reviewed changes

src/Standards/Generic/Sniffs/PHP/DeclareStrictTypesSniff.php Show resolved Hide resolved

gsherwood added the Type: enhancement label Dec 6, 2017

jrfnl reviewed Dec 31, 2017

View reviewed changes

michalbundyra mentioned this pull request Jan 25, 2018

[WIP] Extended Zend Framework Coding Standard zendframework/zend-coding-standard#1

Closed

34 tasks

gsherwood added this to the 3.3.0 milestone Jan 31, 2018

gsherwood mentioned this pull request Mar 15, 2018

Looking for a couple sniffs [question not bug] #1935

Closed

gsherwood removed this from the 3.3.0 milestone Mar 23, 2018

michalbundyra added 4 commits March 25, 2018 15:52

Added new sniff Generic.PHP.DeclareStrictTypes

cac6a67

Allow declare strict types in one line

d17c657

Changed default value for $linesBefore to 1

7f35bf6

DeclareStrictTypesSniff updates

0053c65

michalbundyra force-pushed the feature/declare-strict-types branch from 8330b5e to 0053c65 Compare March 25, 2018 14:56

michalbundyra added 4 commits March 25, 2018 16:13

Renamed properties

3b0645a

- linesBefore -> spacingBefore - linesAfter -> spacingAfter

Simplify code as findEndOfStatement now works correctly

0b073f3

Removed omitCommentWithTags property

b9025ea

Updated tests to cover all cases

855f68c

Use $phpcsFile->numTokens instead of counting tokens

6165428

jrfnl reviewed Mar 27, 2018

View reviewed changes

Returns ($phpcsFile->numTokens + 1) when nothing else to process

6e947af

- per @jrfnl

gmponos reviewed Nov 23, 2018

View reviewed changes

gsherwood closed this Dec 12, 2018

gsherwood reopened this Dec 12, 2018

floverdevel mentioned this pull request Jan 11, 2019

Declare strict types #2365

Closed

gsherwood added this to the 3.5.0 milestone Jan 24, 2019

michalbundyra added 2 commits January 24, 2019 08:04

Changed format property to private

69ad7d0

We don't want this configuration option to be available - per @gsherwood

Updated year in copyright headers

4cd331f

lenaorobei mentioned this pull request Feb 1, 2019

[New Rule] Add declare(strict_types=1) to your PHP files magento/magento-coding-standard#33

Open

michalbundyra closed this Apr 6, 2019

michalbundyra deleted the feature/declare-strict-types branch April 6, 2019 07:41

gsherwood removed this from the 3.5.0 milestone Sep 26, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added new sniff Generic.PHP.DeclareStrictTypes #1771

Added new sniff Generic.PHP.DeclareStrictTypes #1771

michalbundyra commented Dec 6, 2017 •

edited

Loading

jrfnl commented Dec 31, 2017 •

edited

Loading

jrfnl left a comment

jrfnl Dec 31, 2017

jrfnl Dec 31, 2017

jrfnl Dec 31, 2017

Xerkus commented Dec 31, 2017

michalbundyra commented Feb 2, 2018 •

edited

Loading

gsherwood commented Mar 23, 2018

michalbundyra commented Mar 26, 2018

jrfnl Mar 27, 2018

michalbundyra commented Apr 4, 2018

ademarco commented Apr 13, 2018

WarriorXK commented Aug 30, 2018

player259 commented Aug 30, 2018

floverdevel commented Nov 15, 2018

gsherwood commented Nov 15, 2018

floverdevel commented Nov 15, 2018

gmponos left a comment

gsherwood commented Dec 12, 2018 •

edited

Loading

floverdevel commented Jan 8, 2019

gsherwood commented Jan 8, 2019

floverdevel commented Feb 18, 2019

gsherwood commented Apr 3, 2019

michalbundyra commented Apr 6, 2019

Dr10s commented Jul 19, 2022

Added new sniff Generic.PHP.DeclareStrictTypes #1771

Added new sniff Generic.PHP.DeclareStrictTypes #1771

Conversation

michalbundyra commented Dec 6, 2017 • edited Loading

jrfnl commented Dec 31, 2017 • edited Loading

jrfnl left a comment

Choose a reason for hiding this comment

Example 1

Example 2

Example 3

Example 4

Example 5

jrfnl Dec 31, 2017

Choose a reason for hiding this comment

jrfnl Dec 31, 2017

Choose a reason for hiding this comment

jrfnl Dec 31, 2017

Choose a reason for hiding this comment

Xerkus commented Dec 31, 2017

michalbundyra commented Feb 2, 2018 • edited Loading

gsherwood commented Mar 23, 2018

michalbundyra commented Mar 26, 2018

jrfnl Mar 27, 2018

Choose a reason for hiding this comment

michalbundyra commented Apr 4, 2018

ademarco commented Apr 13, 2018

WarriorXK commented Aug 30, 2018

player259 commented Aug 30, 2018

floverdevel commented Nov 15, 2018

gsherwood commented Nov 15, 2018

floverdevel commented Nov 15, 2018

gmponos left a comment

Choose a reason for hiding this comment

gsherwood commented Dec 12, 2018 • edited Loading

floverdevel commented Jan 8, 2019

gsherwood commented Jan 8, 2019

floverdevel commented Feb 18, 2019

gsherwood commented Apr 3, 2019

michalbundyra commented Apr 6, 2019

Dr10s commented Jul 19, 2022

michalbundyra commented Dec 6, 2017 •

edited

Loading

jrfnl commented Dec 31, 2017 •

edited

Loading

michalbundyra commented Feb 2, 2018 •

edited

Loading

gsherwood commented Dec 12, 2018 •

edited

Loading