WordPress / WordPress-Coding-Standards

PHP_CodeSniffer rules (sniffs) to enforce WordPress coding conventions
MIT License
2.54k stars 479 forks source link

Add fixer for TextDomainMismatch #1174

Closed GaryJones closed 5 years ago

GaryJones commented 6 years ago

https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/blob/d62b34082bf1921c0896ac7a0b6f32a85fe9a576/WordPress/Sniffs/WP/I18nSniff.php#L369

(and a repeat a few lines down) identifies when a text domain does not match what it should.

Since we have the text domain string in the function call, and the allowed text domain(s), could a fixer be added when there is only a single intended domain that does the replacement?

jrfnl commented 6 years ago

In my opinion: no. There is no guarantee that the text domains list passed is complete and the fixer would presume it is if we would do this, which I think makes this a risky fix and not suitable for auto-fixing.

GaryJones commented 6 years ago

If one text domain has been set, I think it's fair to assume that this is the config the developer wanted.

jrfnl commented 6 years ago

I still think this would be a very risky fixer:

jrfnl commented 6 years ago

I've been thinking some more about this and while I still think the fixer should not be added to the I18n sniff, I was thinking we could possibly add a new Util category with sniffs containing "risky" fixers which would only be activated when certain properties would be set from a custom ruleset. Without those properties, the sniff(s) would not do anything.

For this particular issue, I was thinking of a sniff which could:

The way I'm thinking it could work would be:

<rule ref="WordPress.Util.I18nFixTextDomain">
    <properties>
        <property name="old_text_domain" type="array">
            <element value="old-text-domain"/>
            <element value="incorrect-text-domain"/>
        </property>
        <property name="new_text_domain" value="my-new-text-domain">
    </properties>
</rule>

@GaryJones What do you think ? If you like the idea, please re-open the issue.

GaryJones commented 6 years ago

Happy to re-open, though I think I disagree with the concept of "risky fixers".

For any competent workflow (those who know enough to be using phpcbf), one would expect that code would be version controlled. As such, any change made by a fixer should be checked before commit, and any undesired changes can be reset / manually fixed.

jrfnl commented 6 years ago

I think I disagree with the concept of "risky fixers".

I didn't come up with it. It's a well known concept.

For any competent workflow (those who know enough to be using phpcbf), one would expect that code would be version controlled. As such, any change made by a fixer should be checked before commit, and any undesired changes can be reset / manually fixed.

Any change should be reviewed, risky or not. Fixers can contains bugs after all. All the same, while I agree that would be ideal, we both know this project operates in the WordPress code space where that is not always the case.