WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

Last modified 4 years ago

#29165 closed enhancement (fixed)

customize_preview_$setting->id action should be customize_preview_$setting->type

Reported by: celloexpressions Owned by: ocean90
Milestone: 4.1 Priority: normal
Severity: normal Version: 3.4
Component: Customize Keywords: has-patch commit
Focuses: Cc:
PR Number:

Description

When working with custom Customizer Setting types, the following actions are available to implement saving and previewing:

  • customize_update_$setting->type
  • customize_preview_$setting->id

Another action, customize_save_$setting->id_data[ 'base' ] fires for all settings, regardless of their type (the inline docs are wrong here, patch fixes them), so it has a different use case (also, the save method calls the update method, which fires the update action). However, customize_preview_$setting->id would be much more useful as customize_preview_$setting->type, matching customize_update_$setting->type, as custom setting types will typically be applicable to multiple settings (ie, multiple ids).

Here's an example of the workaround that I'm using in the Menu Customizer:

function menu_customizer_setup_menu_previewing() {
	$menus = wp_get_nav_menus();

	foreach ( $menus as $menu ) {
		$menu_id = $menu->term_id;
		$setting_id = 'nav_menu_' . $menu_id;
		add_action( 'customize_preview_' . $setting_id, 'menu_customizer_preview_nav_menu', 10, 1 );
	}
}
add_action( 'customize_register', 'menu_customizer_setup_menu_previewing' );

Which could be replaced with, simply:

add_action( 'customize_preview_nav_menu', 'menu_customizer_preview_nav_menu', 10, 1 );

Which would match the corresponding update action:

add_action( 'customize_update_nav_menu', 'menu_customizer_update_nav_menu', 10, 2 );

Can't really change the action at this point, so patch adds a new one. Would love to get this into 4.0, but probably needs to wait until 4.1.

Attachments (3)

29165.diff (1.2 KB) - added by celloexpressions 5 years ago.
Introduce customize_preview_$setting->type action, correct docs for customize_save_ action.
29165.2.diff (851 bytes) - added by celloexpressions 5 years ago.
Add customize_preview_$setting->type action.
29165.3.diff (857 bytes) - added by celloexpressions 5 years ago.

Download all attachments as: .zip

Change History (20)

@celloexpressions
5 years ago

Introduce customize_preview_$setting->type action, correct docs for customize_save_ action.

#1 @celloexpressions
5 years ago

Forgot to mention, this is particularly relevant now that the setting instance is passed to the action (as of 4.0), as the id and other information can be easily retrieved in the callback function.

#2 @celloexpressions
5 years ago

  • Focuses docs added
  • Milestone changed from Future Release to 4.0

Moving to 4.0 for the docs correction, at least, as it is misleading as to the purpose of the customize_save_$setting->id_data['base'] action. The hook addition could slip in now, or be pushed back to 4.1.

This ticket was mentioned in IRC in #wordpress-dev by ocean90. View the logs.


5 years ago

#4 @ocean90
5 years ago

In 29509:

Correct the documentation for the customize_save_* action.

props celloexpressions.
see #29165.

#5 @ocean90
5 years ago

  • Focuses docs removed
  • Milestone changed from 4.0 to Future Release
  • Type changed from defect (bug) to enhancement

#6 @ocean90
5 years ago

Related: #29316

@celloexpressions
5 years ago

Add customize_preview_$setting->type action.

#7 @celloexpressions
5 years ago

  • Milestone changed from Future Release to 4.1

Remaining item is the addition/correction of the hook (original purpose of this ticket). The current setup is pretty silly when you're working with custom setting types.

Should be ready for commit, updated patch refreshed for trunk.

#8 follow-up: @nacin
5 years ago

Is two hooks having the same base of customize_preview_* a collision issue?

#9 in reply to: ↑ 8 @celloexpressions
5 years ago

Replying to nacin:

Is two hooks having the same base of customize_preview_* a collision issue?

I doubt it. You would have to have a setting id that matches the type of another setting. I see that as being pretty rare; the only situation it might happen is if a particular setting has an id that matches its type. But then any filters they add would just run twice, which shouldn't be an issue. But the setting type should refer to the way the data's being stored (theme_mod, option, nav_menu (as in the taxonomy), etc.), whereas the id should be specific to the purpose of a given option.

#10 follow-up: @SergeyBiryukov
5 years ago

Should we make it 'customize_preview_type_' . $this->type, just in case?

#11 in reply to: ↑ 10 @celloexpressions
5 years ago

Replying to SergeyBiryukov:

Should we make it 'customize_preview_type_' . $this->type, just in case?

That wouldn't align with customize_update_$setting->type. customize_update_$type and customize_preview_$type are the two actions that are necessary for pretty much any preview-able custom setting type, whereas all of the other hooks are more appropriate if you're targeting a specific setting instance (by id). They also read as customize_update_nav_menu and customize_preview_nav_menu, which is a lot better than customize_preview_type_nav_menu.

Few enough people are using it, that we could probably just change it and reach out to potentially-affected developers if we're worried about collisions. WordPress.com is likely one of the only ones. This part of the API is pretty severely under-documented.

I did discuss this with @ethitter and @obenland too; they suggested adding the second hook here rather than changing it and worrying about compat.

#12 @celloexpressions
5 years ago

  • Keywords commit added

Unless there are further concerns with conflicts, 29165.2.diff is ready for commit. And worst case scenario, a function might be called twice, but as I outlined above, that's extremely unlikely. Also, given the extremely limited use of custom setting types in the wild currently, and the fact that these actions are only called for custom setting types, we'll definitely hear about it if anyone does run into an issue.

When it comes down to it, this was a typo/oversight when the Customizer was introduced in 4.1.

#13 @DrewAPicture
5 years ago

Two things with 29165.2.diff:

  • Can we please use an interpolated string for the hook name, e.g. "customize_preview_{$this->type}"?
  • Also, could you please wrap the reference to WP_Customize_Setting::preview() in the hook doc description using an inline @see tag? e.g. {@see WP_Customize_Setting::preview()}, that way it'll get auto-linked in the code reference.

I notice that we use concatenation style for most/all of the hooks in this file, though I also notice that in the code reference, customize_preview_' . $this->id shows up as interpolated, so that's interesting ...

#14 @celloexpressions
5 years ago

I literally just copied and pasted the other one, but 29165.3.diff addresses those. Someone should change all of the Customizer files to do that at some point (I think those hooks are mostly for settings though).

This ticket was mentioned in IRC in #wordpress-dev by celloexpressions. View the logs.


5 years ago

#16 @ocean90
5 years ago

  • Owner set to ocean90
  • Resolution set to fixed
  • Status changed from new to closed

In 29948:

Customizer: Introduce customize_preview_$setting->type action to handle multiple settings of the same type.

props celloexpressions.
fixes #29165.

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


4 years ago

Note: See TracTickets for help on using tickets.