Add sniff for declare(strict_types=1);

MR: https://www.drupal.org/project/coder/issues/3407995 ✨ Add sniff for declare(strict_types=1); Needs review

You linked back to this same issue :-)

Here is your PR https://github.com/pfrenssen/coder/pull/217

I have a question on the added line declare(strict_types=1); in test file . I don't think we need that, because declare(strict_types=1); is already added to the file. The comment for good.module says 'It is used to check sniffs that run on .module files but skip .php files' so we are already covered.

Also regarding the tests, I know that its a third-party sniff, but would it be useful to add an incorrect 'declare' and check that it is fixed in the .fixed file? We don't need to do in-depth testing of the sniff, but I think it would be beneficial if our tests showed that it is being applied and the fixer works as expected.

Merged, thanks!

We could be more fancy with the testing, but I think it is ok to rely on upstream Slevomat to do it fully.

Thanks for merging.

I'm fine with your answer to my second question in #4

But did you have an opinion on my first question? That's been committed now, but I don't think that line was needed, and now seems confusing in that file.

I think it is fine to have it also in good.module to mirror what is in bad.module, so can stay as is.

Automatically closed - issue fixed for 2 weeks with no activity.

Note, the changeset introduced here prevents enforcing declare(strict_types) on files.

Per

<exclude name="SlevomatCodingStandard.TypeHints.DeclareStrictTypes.DeclareStrictTypesMissing"/>

in https://git.drupalcode.org/project/coder/-/commit/eb31ae916dcb6221e8783a...

To bring back the requirement, add this line:

    <rule ref="SlevomatCodingStandard.TypeHints.DeclareStrictTypes.DeclareStrictTypesMissing">
        <severity>5</severity>
    </rule>

As discovered in https://github.com/previousnext/coding-standard/pull/82

Hi @dpi, just for clarity, is this a hint for contrib modules who want to enforce the sniff? Or are you saying that instead of <exclude> the change to Coder's ruleset.xml should have been to use <severity>0</severity>? Looking at that file, every other sniff that is not enabled uses severity 0, this is the only one which uses exclude name=

If there is work to be done, then a maintainer will have to re-open this issue, or maybe we start a new one?

its a hint for both contrib, but primarily private projects who extend coder.

Private projects may choose to force strict types, and anyone who did so before this change will suddenly find that even if they have the rule declared, Coders' declaration overrides theirs. So projects and contrib need to make their declaration more specific, in order to override Coders.

Its more a PSA than anything. But, perhaps (unlikely) there was a way for coder to introduce this change without using the exclude directive.

Thanks for the clarity.

But, perhaps (unlikely) there was a way for coder to introduce this change without using the exclude directive.

Would using severity 0instead of exclude have been better for this situation?

Didnt work.

I think somewhere behind the scenes, in PHPCS, exclude translate to a regular <rule ...><severity>0 declaration.

Didnt work.

What didn't work? Do you mean that you tried the suggested change to Coder's ruleset.xml in #12 and #14 and it was still not any better for your situation?

exclude didnt work. Like I said, it seems to just translate to a 0 declaration under the hood.