WordPress / WordPress-Coding-Standards

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

Handbook: Add sniff(s) for "Interpolation for Naming Dynamic Hooks" section #751

Open jrfnl opened 7 years ago

jrfnl commented 7 years ago

Dynamic hooks should be named using interpolation rather than concatenation for readability and discoverability purposes.

Dynamic hooks are hooks that include dynamic values in their tag name, e.g. {$new_status}_{$post->post_type} (publish_post).

Variables used in hook tags should be wrapped in curly braces { and }, with the complete outer tag name wrapped in double quotes. This is to ensure PHP can correctly parse the given variables’ types within the interpolated string.

do_action( "{$new_status}_{$post->post_type}", $post->ID, $post );

Where possible, dynamic values in tag names should also be as succinct and to the point as possible. $user_id is much more self-documenting than, say, $this->id.

Ref: https://make.wordpress.org/core/handbook/best-practices/coding-standards/php/#interpolation-for-naming-dynamic-hooks

jrfnl commented 7 years ago

This should probably be added to the WordPress.NamingConventions.ValidHookName sniff. https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/blob/develop/WordPress/Sniffs/NamingConventions/ValidHookNameSniff.php

JDGrimes commented 7 years ago

On a related note, when interpolating variables I find it better to use a hyphen as a separator instead of an underscore:

hook_name-{$dynamic_part} vs hook_name_{$dynamic_part}.

Using an underscore can cause the hook to clash with other dynamic/static hooks with a similar prefix under some circumstances. E.g., it would clash with a hook named hook_name_static_part, if $dynamic_part could ever have the value 'static_part'.

Anyway, the point is, I generally use a - between the dynamic portion of the sniff and the static portion, in order to avoid this possibility. Currently the sniff will flag that with an error, however.

396 | WARNING | Words in hook names should be separated using
     |         | underscores. Expected: 
     |         | "wordpoints_modules_status_link_text_{$type}", but found: 
     |         | "wordpoints_modules_status_link_text-{$type}".
     |         | (WordPress.NamingConventions.ValidHookName.UseUnderscores)

I'd like to consider possibly making allowances for hyphens around dynamic hook portions in this sniff in the future.

jrfnl commented 7 years ago

I'd like to consider possibly making allowances for hyphens around dynamic hook portions in this sniff in the future.

While not specific to dynamic hook portions, you already can provide custom word separators to the hook name sniff. The below will allow for the - as well as the default underscore word separator:

<rule ref="WordPress.NamingConventions.ValidHookName">
    <properties>
        <property name="additionalWordDelimiters" value="-"/>
    </properties>
</rule>
samwilson commented 7 years ago

I'm not sure how related this is, but I'm currently getting a phpcs error WordPress.NamingConventions.ValidHookName.UseUnderscores for activation hooks which are created in register_activation_hook() with concatenation and include dots and slashes because they include filenames:

For example, when the plugin is located in wp-content/plugins/sampleplugin/sample.php, then the name of this hook will become 'activate_sampleplugin/sample.php'.

Is there a recommended fix for this?

jrfnl commented 7 years ago

@samwilson This is unrelated. This issue is about a part of the core coding standards which isn't covered yet. Your issue is about something which is covered. Please open a separate issue about this.

samwilson commented 7 years ago

Thanks @jrfnl, I've opened #881.