CapitalPDangit doesn't prevent matches on reverse domain name notation

[mirka]

Bug Description

The CapitalPDangit sniff can correct case-sensitive identifiers in reverse domain name notation (e.g. com.wordpress).

Minimal Code Snippet

<?php
$stat_id = 'com.wordpress.signups';

Error Code

WordPress.WP.CapitalPDangit.Misspelled

Environment

Question	Answer
PHP version	7.3.11
PHP_CodeSniffer version	3.5.5
WPCS version	2.3.0
WPCS install type	Composer project local

Additional Context (optional)

Similar issue to #1698

I understand it's unreasonable to expect this sniff to prevent every single false positive, but just flagging this particular case since it is a potentially bug-inducing autofix 🙂

Tested Against develop branch?

• [ ] I have verified the issue still exists in the develop branch of WPCS.

[jrfnl]

@mirka Could you give me some context about the use of these "reverse domain name" notations ? AFAIK this is not something which is supported by WP by default or am I missing something ?

[mirka]

Yes, that’s probably correct, I don’t imagine the WP Core codebase uses notation like this anywhere. This is just an arbitrary identifier that was in our team’s codebase.

If it matters, they were IDs for tracking stats, and they had reverse-DNS prefixes for namespacing.

https://github.com/WordPress/WordPress-Coding-Standards/blob/ac0a94c2f831b9adb633e7af5f000d6a0df650f9/WordPress/Sniffs/WP/CapitalPDangitSniff.php#L244-L256

^ I saw that there was already some false positive prevention code like this. But I wasn’t sure if these were specifically meant for exceptions that could occur within the WP Core code, or for broader applications as well. If it’s the former, and this case is not worth preventing specifically, that’s totally reasonable!

[jrfnl]

@mirka The false positive prevention code currently in place is for things which regularly occur in codebases, not for individual usecases.

For the time being, I'd like to suggest for you to use the standard PHPCS ignore annotations to prevent the issue being reported and the auto-fixer from changing the code:

// phpcs:ignore WordPress.WP.CapitalPDangit.Misspelled -- Reverse DNS notation.
$stat_id = 'com.wordpress.signups';

I'll leave this issue open for now to see if this is something used more frequently than just your usecase. If so, it could be added to the false positive prevention.

So to anyone out there using reverse DNS notation with on wordpress.org/com: please leave a comment.

[theenoahmason]

This issue just drove me crazy for a bit! Posting here in case someone else lands on the same issue.

I have a list of tinyMCE plugins as a string: 'charmap,colorpicker,hr,lists,media,paste,tabfocus,textcolor,fullscreen,wordpress,wpautoresize,wpeditimage,wpemoji,wpgallery,wplink,wpdialogs,wptextpattern,wpview', one of which is wordpress.

phpcs lights this up as a warning and phpcbf fixes it (breaking some functionality of tinyMCE).

Note: // phpcs:ignore WordPress.WP.CapitalPDangit Does not prevent the issue from happening. The only way I got around it is by turning off the rule in my phpcs xml file.

[iandunn]

More broadly, I ran across an instance today where phpcbf caused a fatal error because it included the wrong file:

function main() {
	$tld = get_top_level_domain();

	switch ( DOMAIN_CURRENT_SITE ) {
		case "events.wordpress.$tld":
			require __DIR__ . '/sunrise-events.php';
			break;

		case "wordcamp.$tld":
		default:
			require __DIR__ . '/sunrise-wordcamp.php';
			break;
	}
}

I took a quick glance at the plugin directory and noticed many other instances where wordpress was used in logic.

Personally, I'd lean towards only checking comments, because variables/hardcoded strings/etc seem too unpredictable to me. Related #1698.

If that's not desired, though, is there a way to configure a sniff to warn but not fix? If so, that might be worth considering in this case.

[jrfnl]

Personally, I'd lean towards only checking comments, because variables/hardcoded strings/etc seem too unpredictable to me. Related https://github.com/WordPress/WordPress-Coding-Standards/issues/1698. If that's not desired, though, is there a way to configure a sniff to warn but not fix? If so, that might be worth considering in this case.

@iandunn The sniff only throws warnings, so that part doesn't need any adjustment. It also doesn't check variables and has a lot of code included to try and prevent these type of false positives already, like not checking constant declarations, preventing false positives on email addresses, directory paths etc.

To make the sniff only report, but not auto-fix for text strings and comments:

 <rule ref="WordPress.WP.CapitalPDangit.Misspelled">
  <severity phpcbf-only="true">0</severity>
 </rule>

Ref: https://github.com/squizlabs/PHP_CodeSniffer/wiki/Annotated-Ruleset#selectively-applying-rules

Just thinking: would it help if we would split the Misspelled error code into two error codes ? MisspelledInComment and MisspelledInText ? That way you could exclude either one of them completely from your custom ruleset.

[jrfnl]

@iandunn Could you have a look at PR #2257 ? It basically applies my suggestion about splitting the error code in two.

That way, you could still let comments be auto-fixed, while you could apply the above ruleset tweak only to text strings.

[iandunn]

I think that's helpful, thanks!