Make WordPress Core

Opened 8 years ago

Closed 8 years ago

Last modified 4 years ago

#37748 closed enhancement (fixed)

Dynamic Hooks Naming Convention

Reported by: ramiy's profile ramiy Owned by:
Milestone: 4.7 Priority: normal
Severity: normal Version:
Component: General Keywords: has-patch
Focuses: docs, performance Cc:

Description

Currently, WordPress core has no naming-convention for dynamic hooks. I'm talking about actions and filters which have parameters in their name.


Proposed Change

Dynamic hooks like:

do_action( 'wp_hook_' . $dynamic_field . '_name' );

Should be replaced with:

do_action( "wp_hook_{$dynamic_field}_name" );


New Naming Convention

Currently, we use both ways to concatenate dynamic fields. We need to have a single standard way to do that.

When hook names have dynamic fields, we should use "wp_hook_{$dynamic_field}_name". It's more readable structure for developers and has performance benefits with multiple dynamic fields, as PHP is forced to re-concatenate with every '.' operator.


Attachments (3)

37748.patch (6.0 KB) - added by ramiy 8 years ago.
37748.2.patch (12.9 KB) - added by ramiy 8 years ago.
37748.3.patch (23.6 KB) - added by ramiy 8 years ago.

Download all attachments as: .zip

Change History (13)

@ramiy
8 years ago

@ramiy
8 years ago

#1 follow-up: @peterwilsoncc
8 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to wontfix
  • Status changed from new to closed

Current practice is for WordPress to avoid refactoring code for such cases as it makes it more difficult to see the history of the project.

Dynamically named hooks are also a thing of the past because they make searching more difficult for developers, as you say.

For these reasons I'll close this wontfix.

#2 @ramiy
8 years ago

@peterwilsoncc Not sure why you closed the ticket - the hook name is not changing.

Currently, hooks that use 'wp_hook_' . $dynamic_field . '_name' structure are already presented as "wp_hook_{$dynamic_field}_name" in the code reference.

The change won't affect the history. And won't affect the search in the code reference. the WordPress-phpDoc-Parse has a very smart script, it converts dynamic hooks artificially (see this hook in code reference). Applying the patch won't affect the code reference and the search.

Besides, it has performance benefits and it sets a new conventions. Those are good things.

Can we at least discuss about the proposal before closing the ticket? I will love to hear @DrewAPicture opinion, and the docs team, about the change.

#3 in reply to: ↑ 1 @DrewAPicture
8 years ago

  • Milestone set to Awaiting Review
  • Resolution wontfix deleted
  • Status changed from closed to reopened

Replying to peterwilsoncc:

Current practice is for WordPress to avoid refactoring code for such cases as it makes it more difficult to see the history of the project.

I actually think there's merit in ignoring the refactoring rule in this instance. Standardizing on interpolation or concatenation would be a good step toward improving these hooks' ability to self document.

Typically, concatenation is going to be faster unless there's multiple parts, at which point the performance gain is largely negated by full concatenations happening for each successive part.

Personally, I prefer interpolation simply because it's easier to read linearly, and we're all about self-documenting code here.

As for whether they're a thing of the past, we introduced four new dynamic hooks in 4.6 alone ;)

Dynamically named hooks are also a thing of the past because they make searching more difficult for developers, as you say.

I think with the introduction of dynamic hook aliases (probably in 4.7, if I can get my act together) will help a lot with search.

This ticket was mentioned in Slack in #core by drew. View the logs.


8 years ago

@ramiy
8 years ago

#5 follow-up: @DrewAPicture
8 years ago

  • Keywords has-patch added

Hook aliases spreadsheet I started a year ago, for anyone interested.

The spreadsheet is a little out of date for new hooks added in the last year, but the gist is to document common aliases of dynamic hooks right in the DocBlocks (and therefore in the Code Reference).

So searching for publish_post in core would actually point you to {$new_status}_{$post->post_type} hook doc. If searching it on the Code Reference, you'd be taken to the corresponding reference page.

I should probably write a post on make/docs to get this started. No time like the present!

#6 in reply to: ↑ 5 @ramiy
8 years ago

Replying to DrewAPicture:

Hook aliases spreadsheet I started a year ago.

The spreadsheet has 118 hooks listed. The code reference say's we have 280 dynamic hooks.

No time like the present!

Lets do it in 4.7, I can help.

#7 @DrewAPicture
8 years ago

In 38307:

Hooks: Standardize naming of dynamic hooks to use interpolation vs concatenation.

Benefits gained in discoverability and self-documentation throughout core trump the negligible performance hit in using interpolation in hook names.

Props ramiy.
See #37748.

#8 @johnbillion
8 years ago

I approve of this change.

#9 @DrewAPicture
8 years ago

  • Milestone changed from Awaiting Review to 4.7
  • Resolution set to fixed
  • Status changed from reopened to closed

Added a Interpolation for Naming Dynamic Hooks section to the Core Coding Standards.

This ticket was mentioned in Slack in #docs by ramiy. View the logs.


4 years ago

Note: See TracTickets for help on using tickets.