WordPress.org

Make WordPress Core

Opened 5 years ago

Last modified 3 months ago

#18857 accepted defect (bug)

get_plugin_page_hookname uses menu_title to construct subpage load-hooks

Reported by: apocalip Owned by: SergeyBiryukov
Milestone: Future Release Priority: normal
Severity: normal Version: 3.1.4
Component: Plugins Keywords: has-patch early needs-unit-tests
Focuses: administration Cc:

Description

The load-hook for PluginSubPages isn't working anymore if the PluginPage is translated.

The reason seems to be that the get_plugin_page_hookname function uses the menu_title instead of the menu_slug to create the hookname.

I attached a possible fix.

Attachments (4)

categories_load_hook_fix.diff (635 bytes) - added by apocalip 5 years ago.
plugin.diff (274 bytes) - added by apocalip 5 years ago.
18857.diff (489 bytes) - added by scribu 4 years ago.
corrrect format of plugin.diff
18857.jpg (126.4 KB) - added by SergeyBiryukov 10 months ago.

Download all attachments as: .zip

Change History (31)

@apocalip
5 years ago

#1 @nacin
5 years ago

  • Keywords dev-feedback added

http://core.trac.wordpress.org/attachment/ticket/18857/categories_load_hook_fix.diff is an attachment for the Shopp plugin, not core. You'll have to file it with them.

For the core fix, I've never noticed this. Nasty.

#2 @SergeyBiryukov
4 years ago

#23297 was marked as a duplicate.

#3 @SergeyBiryukov
4 years ago

  • Component changed from General to Plugins

#4 @SergeyBiryukov
4 years ago

  • Milestone changed from Awaiting Review to 3.6

#5 @brasofilo
4 years ago

  • Cc brasofilo@… added

#6 @scribu
4 years ago

  • Keywords has-patch added; dev-feedback removed

@scribu
4 years ago

corrrect format of plugin.diff

#7 @SergeyBiryukov
4 years ago

#21454 was marked as a duplicate.

#8 @SergeyBiryukov
4 years ago

#24274 was marked as a duplicate.

#9 @jrfoell
4 years ago

  • Cc justin@… added

#11 @SergeyBiryukov
4 years ago

Introduced in [2234].

#12 @toscho
4 years ago

  • Cc info@… added

#13 @ryan
4 years ago

  • Milestone changed from 3.6 to Future Release

#14 @nacin
3 years ago

  • Component changed from Plugins to Admin APIs
  • Focuses administration added

#15 @nacin
3 years ago

  • Component changed from Admin APIs to Plugins

Sorry for the noise.

#16 @ocean90
3 years ago

#27732 was marked as a duplicate.

#17 @SergeyBiryukov
2 years ago

#30073 was marked as a duplicate.

#18 @nerrad
2 years ago

any reason why @scribu 's patch is not being used?

#19 @bobbingwide
21 months ago

+1 for scribu's fix.

add_menu_page() returns the hook_suffix to use if you want to respond to "load-$hook_suffix", "admin_head-$hook_suffix", etc.

But you can't trust it since the value of $hook_suffix is partially localized, if the $menu_title parameter has been localized.

It is therefore conceivable that the $hook_suffix returned would be identical to one that has already been returned, leading to unexpected processing.

The translator would be totally unaware of the problems they could cause.

Unlikely example.

  $hook = add_submenu_page( 'menu_18857', __( "Pound euro", 'domain' ), __( "Pound euro", 'domain' ), 'manage_options', 'menu_18857_poundeuro', "menu_18857_pound_euro" );
  add_action( "load-$hook", "menu_18857_poundeuro_load" );
	
  $hook2 = add_submenu_page( 'menu_18857', __( "lower case e", 'domain' ), __( "lower case e", 'domain' ), 'manage_options', 'menu_18857_lowercasee', "menu_18857_lower_case_e" );
  add_action( "load-$hook2", "menu_18857_lowercasee_load" );



If the translator decides "Pound euro" should be "£€", and "lower case e" should be "e" then
sanitize_title( $menu_title ) will return "e" for both menu items.

So $hook and $hook2 will have the same value, leading to unexpected processing;
both action hooks menu_18857_pound_euro and menu_18857_lowercasee_load being executed when either menu item is selected.

In short, $menu_title should not be used to determine the $hook_suffix.

Alternative to scribu's fix.

 $admin_page_hooks[$menu_slug] = sanitize_title( $menu_slug );

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


21 months ago

#21 follow-up: @SergeyBiryukov
10 months ago

  • Keywords early added
  • Milestone changed from Future Release to 4.6

Just stumbled upon this issue with iThemes Security (thanks to @fierevere for the report).

The plugin adds a submenu page:

$this->available_pages[] = add_menu_page(
	__( 'Dashboard', 'better-wp-security' ),
	__( 'Security', 'better-wp-security' ),
	$itsec_globals['plugin_access_lvl'],
	'itsec',
	array( $this, 'render_page' )
);

and then uses screen IDs like security_page_toplevel_page_itsec_settings to display settings, meta boxes, etc.

However, if __( 'Security', 'better-wp-security' ) is translated, screen ID changes too, e.g. if the string is translated into "Безопасность" in ru_RU, the screen ID becomes %d0%b1%d0%b5%d0%b7%d0%be%d0%bf%d0%b0%d1%81%d0%bd%d0%be%d1%81%d1%82%d1%8c_page_toplevel_page_itsec_settings, and the corresponding settings page becomes blank: 18857.jpg. See #30073 for a detailed explanation.

Let's see what is needed to get 18857.diff in.

Last edited 10 months ago by SergeyBiryukov (previous) (diff)

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


10 months ago

#23 in reply to: ↑ 21 @ocean90
10 months ago

  • Keywords needs-unit-tests added

Replying to SergeyBiryukov:

Let's see what is needed to get 18857.diff in.

Probably just a unit test so it doesn't happen again and we can verify that it fixes the issue?

This ticket was mentioned in Slack in #polyglots by yui. View the logs.


9 months ago

#25 @ocean90
8 months ago

  • Milestone changed from 4.6 to Future Release

Punting since the ticket is tagged with early and unit tests are still missing.

#26 @SergeyBiryukov
6 months ago

  • Owner set to SergeyBiryukov
  • Status changed from new to accepted

#27 @ocean90
3 months ago

#38814 was marked as a duplicate.

Note: See TracTickets for help on using tickets.