Add new sniff to check whether a hook name is valid.

[jrfnl]

Covers the action part of this rule in the handbook:

Use lowercase letters in variable, action, and function names (never camelCase). Separate words via underscores.

Ref: https://make.wordpress.org/core/handbook/best-practices/coding-standards/php/#naming-conventions

Review appreciated.

Also: I've made non-lowercase hook names an error and non-underscore punctuation a warning. Should that also be an error ?

[GaryJones]

Slight aside from here, but maybe worth adding as a valid unit test to check the new sniff doesn't complain, is hook names with / in them, like acf/save_post.

[jrfnl]

adding as a valid unit test to check the new sniff doesn't complain, is hook names with / in them

@GaryJones That would throw a warning based on the below phrase in the rule as stated in the handbook:

Separate words via underscores.

I choose warning for that part as core often doesn't comply with this rule either - they often use dashes instead of underscores - and changing hook names would break backward compatibility.

[westonruter]

See also humanmade/WordPress-Importer#10 (comment)

[jrfnl]

@westonruter Same goes there - it would throw a warning, not an error.

[jrfnl]

Rebased to force a travis check against the new WPCS codestyle rules.

[GaryJones]

It's not so much a separate word, as a prefix.

[JDGrimes]

I wonder if core is actually strict about this rule. Personally I like to use a hyphen or something other than an underscore in dynamic hook names to prevent potential conflicts:

// These could conflict unexpectedly:
do_action( "prefix_{$var}" );
do_action( 'prefix_something' );

// This prevents that:
do_action( "prefix-{$var}" );

But if core is going to be strict about this, then I guess we do need to add this check to core, and if some of us don't want to use it we can silence it.

[jrfnl]

I wonder if core is actually strict about this rule. Personally I like to use a hyphen or something other than an underscore in dynamic hook names to prevent potential conflicts:

I had a look at this hook database when I created the sniff and based on visual inspection, it seemed that core is strict about the lower case - only 13 exceptions out of 2000+ hook names AFAICS.
Exceptions I found were:

• attachment_innerHTML
• xmlrpc_call_success_blogger_deletePost up to xmlrpc_call_success_wp_newComment (12 hooks around line 2088)

Core, however, is inconsistent about applying the second part of the rule about using _ for punctuation. Dashes are most frequently used as an alternative, often even combined with underscores within the same hook name. Often you'll find this is when they refer to a function name within the hook - the 'words' within the function name are then separated with dashes while the other 'words' are separated with underscores.

All the more reason why I made a difference in error levels between the two parts. Error for non-lowercase, Warning for non-underscore punctuation.

[jrfnl]

Just thinking - I could add an option for people to provide one-character punctuation exceptions via the phpcs.xml config.

What do you think ?

[jrfnl]

New commit added - this creates the ability to provide additional allowed word delimiters via phpcs.xml or via the inline @codingStandardsChangeSetting directive.

Example usage:

<rule ref="WordPress.NamingConventions.ValidHookName">
   <properties>
     <property name="additionalWordDelimiters" value="-"/>
   </properties>
</rule>

Commit includes unit tests.

I've still left the error level for the punctuation at warning or should this now be raised to error ?

[GaryJones]

This break, and the break below are redundant (dead code).

[jrfnl]

Good point. Artifact left from upstream code style. Removed.

[jrfnl]

Rebased to solve merge conflict.

[jrfnl]

Anyone got anymore feedback on this ?