PHP templates-specific sniffs

[shvlv]

Please check if the PR fulfills these requirements

• The commit message follows our guidelines
• Tests for the changes have been added (for bug fixes/features)
• Docs have been added/updated (for bug fixes/features)

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Feature.

What is the current behavior? (You can also link to an open issue here)
There are no rules targeted to the PHP templates.

What is the new behavior (if this is a feature change)?

PHP templates have specific formatting rules. For some of our projects, we follow Plates-suggested syntax . It includes using alternative syntax for control structures, short echo tags, and avoiding semicolons. Therefore, creating a set of rules targeted to PHP templates makes sense.

The current PR introduces the InpsydeTemplates ruleset that could be used conditionally (or not used 😄 ) in your projects:

<rule ref="InpsydeTemplates">
    <include-pattern>*/templates/*</include-pattern>
    <include-pattern>*/views/*</include-pattern>
</rule>

For now, only one rule is added to remove trailing semicolons before closing PHP tags. A very nice investigation and usage comparison of this rule can be found here: prettier/plugin-php#609 (comment).

Sure, we can and should discuss the approach widely. Maybe we could decide to maintain such rules in a separate repository. But since we already have a test suite and documentation here, in my opinion, it will be no harm to add custom rules that could be enabled conditionally.

Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)

No.

Other information:

Apart from the feature, there is a small deprecation (since PHPCS 3.8.0) fix here -

php-coding-standards/tests/cases/FixturesTest.php

Lines 227 to 231 in 441f7ce

	$ruleset->setSniffProperty(
	$sniffFqn,
	$name,
	['scope' => 'sniff', 'value' => $value],
	);

. It could be applied separately.

[tyrann0us]

Thank you for adding the new ruleset and sniff! I haven't tested the sniff itself, but only left two minor comments in the README.md. Thanks for working on this! 💪🏽

[tfrommen]

Thanks for working on this!

I left a few comments, but none of them are blocking.

The new section in the readme (and how the new standard relates to the existing Inpsyde one) makes it clear how to use it. But introducing a new standard specific to templates and connecting it to the existing broader standard in a specific way (making the new one extend the existing one) are two things. Where did people discuss how to relate the two standards? What were the reasons for the current approach?

I am only raising this because there are at least three ways to go about this:

• InpsydeTemplates is a new, separate standard, and the existing Inpsyde one includes them (so that people can turn it off for specific contexts). This is how the WordPress and, say, WordPress-Docs standards are related to each other.
• InpsydeTemplates extends Inpsyde. This is the current setup.
• Inpsyde and InpsydeTemplates are completely decoupled and unrelated. People would then decide for what paths they want which standard(s) to be used.

No one of them is "better" than the others. They are different, allow for different things, and require different "effort" when using them.

Wouldn't it be good to have that discussion or documentation (if the discussion already happened) here in this very PR?

[shvlv]

Wouldn't it be good to have that discussion or documentation (if the discussion already happened) here in this very PR?

Thanks for pointing this out. The PR was updated after an internal discussion, but it does make sense to document the decision and explain the reasons behind it.

Inpsyde and InpsydeTemplates standards and their relationship

I initially suggested making the InpsydeTemplates coding standard completely decoupled and unrelated (as you can see from the PR description). In this case, even using the standalone repository is possible (but it's not the best idea, as we have a testing framework and primary PHP coding standard documentation here). Also, PHP templates are still regular PHP files requiring other code-quality sniffs. So, we must apply both Inpsyde and InpsydeTemplates sniffs to PHP templates.

It's possible with the decoupled setup like:

<?xml version="1.0" encoding="UTF-8"?>
<ruleset>
    <file>./src/</file>
    <file>./tests</file>
    <file>./functions.php</file>
    <file>./views</file>

    <rule ref="Inpsyde">
    </rule>

    <rule ref="InpsydeTemplates">
        <include-pattern>*/views/*</include-pattern>
    </rule>

</ruleset>

But what if we want turn off some rules (like NoElse) from Inpsyde in the template context? Sure, we can do it in our project-specific standard, but we can't do it in InpsydeTemplates by default.

That's why the "InpsydeTemplates extends the Inpsyde coding standard" option was selected. It provides maximum flexibility (we can conditionally turn off Inpsyde rules, and we can add third-party template-specific sniffs, if any) and not add much verbosity to consuming package config:

<?xml version="1.0" encoding="UTF-8"?>
<ruleset>
    <file>./src/</file>
    <file>./tests</file>
    <file>./functions.php</file>
    <file>./views</file>

    <rule ref="Inpsyde">
        <exclude-pattern>*/views/*</include-pattern>
    </rule>

    <rule ref="InpsydeTemplates">
        <include-pattern>*/views/*</include-pattern>
    </rule>
</ruleset>

Regarding "Inpsyde including InpsydeTemplates", it might work as well. Basically, it's the inversion of the initial suggestion:

 <?xml version="1.0" encoding="UTF-8"?>
<ruleset>
    <file>./src/</file>
    <file>./tests</file>
    <file>./functions.php</file>
    <file>./views</file>

    <rule ref="Inpsyde">
    </rule>

    <rule ref="InpsydeTemplates">
        <exclude-pattern>*/src/*</include-pattern>
        <exclude-pattern>*/tests/*</include-pattern>
        <exclude-pattern>*/functions.php</include-pattern>
    </rule>
</ruleset>

But most of the time, it will be more verbose. And still, turning off specific rules from Inpsyde in a template context is not possible.

Last but not least, consider backward compatibility. Changing the Inpsyde coding standard in this way will require updating every project-specific standard.

Based on the listed reasons, the "InpsydeTemplates extends the Inpsyde coding standard" seems the most reasonable.

Would it be possible not to create the standalone standard but add a new sniffs directory?

Let's consider the following structure.

Sniffs
    CodeQuality
    Templates

Implementing the selected setup is not possible. Any imported sniffs from PSR, WordPress, etc., in the following ruleset are ignored. Only the CodeQuality and Templates sniffs are imported:

<?xml version="1.0" encoding="UTF-8"?>
<ruleset>
    <file>./src/</file>
    <file>./tests</file>
    <file>./functions.php</file>
    <file>./views</file>

    <rule ref="Inpsyde.CodeQuality">
        <exclude-pattern>*/views/*</include-pattern>
    </rule>

    <rule ref="Inpsyde.Templates">
        <include-pattern>*/views/*</include-pattern>
    </rule>
</ruleset>

We have to import the whole Inpsyde standard to import external sniffs. So it equals "Inpsyde including InpsydeTemplates" with discussed already cons (it is impossible to turn off curated Inpsyde rules, it is impossible to include third-party template-specific rules, it breaks backward compatibility and might be more verbose):

<?xml version="1.0" encoding="UTF-8"?>
<ruleset>
    <file>./src/</file>
    <file>./tests</file>
    <file>./functions.php</file>
    <file>./views</file>

    <rule ref="Inpsyde">
    </rule>

    <rule ref="Inpsyde.Templates">
        <exclude-pattern>*/src/*</include-pattern>
        <exclude-pattern>*/tests/*</include-pattern>
        <exclude-pattern>*/functions.php</include-pattern>
    </rule>
</ruleset>

[gmazzap]

Sorry, by mistake I pushed in this branch instead of development. But I changed nothing on this PR's files, only fixed Psalm issues in other files.