get_plugin_page_hookname uses menu_title to construct subpage load-hooks

[apocalip]

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.

[nacin]

​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.

[SergeyBiryukov]

#23297 was marked as a duplicate.

[scribu]

corrrect format of plugin.diff

[SergeyBiryukov]

#21454 was marked as a duplicate.

[SergeyBiryukov]

#24274 was marked as a duplicate.

[ryan]

#12101 [12914]

[SergeyBiryukov]

Introduced in [2234].

[nacin]

Sorry for the noise.

[ocean90]

#27732 was marked as a duplicate.

[SergeyBiryukov]

#30073 was marked as a duplicate.

[nerrad]

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

[bobbingwide]

+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 );

[SergeyBiryukov]

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.

[ocean90]

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?

[ocean90]

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

[ocean90]

#38814 was marked as a duplicate.

[sk.shaikat]

Replying to apocalip:

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.

​https://github.com/WordPress/WordPress/pull/301

[davilera]

#13266 was marked as a duplicate.

[davilera]

Patch 18857.2.diff​ applies the fix proposed by @scribu and includes a unit test, as requested.

[pento]

Let's try this in 5.2. Patch needs reviewing for side effects and back compat issues.

[chriscct7]

I went to go punt this and noticed Pento beat me to it.

As an update I took a lot of time on this with one of our company developers and we thought through a lot of the edge cases.

Unfortunately, from having done a lot of hours of testing and research, I don't think this is going to be possible to fix moving forward a without backwards compatibility break, specifically the one the patch would make. I would suggest those breaks still be made, but that this goes in first thing into 5.2, and it be announced to plugin developers straightaway so they have the entire cycle to make a very quick addition. Sadly while this would have been very easy to address & fix 7 years ago, doing so now is no longer as straightforward.

I will put together a summary of the research tomorrow but basically the issue comes down to the fact that the best practice of allowing translations for menus has lead to a situation that it will be impossible to fix perfectly (either core will need to break BC, which I would suggest, or we will have to build a new option that new plugins can opt into to fix it moving forward, which I wouldn't recommend since all plugins using add_menu_page would also have to know to use it moving forward which is definitely not dev friendly).

I will outline everything we looked into, the scope of possible changes we could do, and so forth as soon as possible.

[chriscct7]

So the problem here is more difficult than the patch represents, and unfortunately would likely have been far simpler to resolve when this ticket was opened than now.

The issue, to recap, is that unlike in the main menu pages, the submenu pages use the Menu Title parameter not the Menu Slug parameter. This is a problem for developers, because most developers do not pass a static string (the menu title is generally passed through a translation filter), and as a result cannot be directly compared against to see if the page you are currently on matches an expected string easily. See comment:21 for a demonstration of this effect. This also as mentioned breaks the admin-page hooks that utilize it in the name of the hook (as shown in comment:19). I'm going to ignore this second issue for the remainder of this comment, though it still exists, so I don't have to duplicate so much (but the same exact issues, benefits and drawbacks apply there as well).

This is notably different from the main pages, which use the slug. This behavior should have been copied as well over to the submenu pages, when this ticket was introduced initially (as the 18857.2.diff patch does), as it would have cleanly and easily fixed this, by allowing for developers to have a simple, static string to compare against.

Notably, in PHP (though not in a non-generated CSS file) a correct comparison can be made by comparing the current screen ID to that of sanitize_title( __( $menu_title, 'your-textdomain-here' ) ) as the prefix for the screen's ID, however this is not only unnecessarily difficult, but also an unexpected bug that a developer is not likely going to be able to immediately recognize (particularly if they are developing the plugin in the same language as the the one present OR if there is no translation pack yet pushed for their plugin for a language that would demonstrate it). It's an unexpected bug that can "occur out of nowhere" if someone translates a plugin and then WordPress.org ships a translation pack for it that "breaks" the condition.

In an ideal world, 18857.2.diff patch would produce the string that would currently be put out if no translation is present. This would allow plugins that have hardcoded this string comparison and either have never run into this issue due to lack of translation, or lack of testing, or not allowing their menu title to be translated, to continue working.

However, because the menu slug can differ from the menu title, something the included 18857.2.diff patch does not unit test for, this can cause a breaking change. For example, in the case where the Menu slug is set to let's say "frontend-scripts" and the title is set to (regardless if this string is translatable or not) Frontend CSS and JavaScript, simply because these are different end result strings after you run them each through sanitize_title, the 18857.2 patch can break different plugins who are maybe not yet experiencing this bug (lack of translation packs or lack of ability to translate the menu title), since it will change the produced string.

In order to fix this in a 100% backwards compatible way, you must be able to take the translated title and "untranslate it" back to the original title. This is the only way to be able to able to get the same exact string each and every time, in a way that produces the same exact string if translations are bypassed for existing plugins. The second half is important because it would prevent the need for WordPress plugins to have to issue an update that includes a WordPress version check.

To just fix this moving forward for plugins (where plugins doing screen->ID checks would have to issue an update; though notably they could be broken already if they are translating the menu page AND have an active translation that is a different string than the input), which is the easiest option to do at this point 18857.2.diff would need to be used, and a lot of heads up time should be given to plugin authors. It is straightforward to detect most of the plugins who would be affected, so there's a possibility those affected authors could be contacted. At the very least, those plugins would need to either:
A) ship a new version of their plugin with an updated string in the string comparisons (or in the use of the admin hooks) and require/support only WordPress 5.2 or newer
B) ship a new version of their plugin with a WordPress version comparison that uses the earlier in my comment proposed code for the hook/screen->ID prefix for older WordPress installs, and the new static string for the new WordPress version (or you could also use the same code previously mentioned for both cases).

The upside of this would be that the only effort required for new plugins would be if they wanted to support older WordPress installs, and that existing installs would only need a quick, easily documented (make core post) code adjustment.

The downside is that plugins that use a statically defined comparison for the admin hooks or string comparison for current screen ID would break unless they did that update.

[jorbin]

As 5.2 is now in beta and this was something that needed early attention, I'm removing it from the milestone.

[ocean90]

#47805 was marked as a duplicate.

[davidbaumwald]

This ticket needs a thorough review and decision. With version 5.3 Beta 1 three days away, this is being moved to Future Release.

[zareidev]

#56460 was marked as a duplicate.

[SergeyBiryukov]

#57022 was marked as a duplicate.

[swissspidy]

#57079 was marked as a duplicate.