WordPress.org

Make WordPress Core

Opened 8 years ago

Last modified 9 days ago

#18857 assigned defect (bug)

get_plugin_page_hookname uses menu_title to construct subpage load-hooks

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

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 (5)

categories_load_hook_fix.diff (635 bytes) - added by apocalip 8 years ago.
plugin.diff (274 bytes) - added by apocalip 8 years ago.
18857.diff (489 bytes) - added by scribu 7 years ago.
corrrect format of plugin.diff
18857.jpg (126.4 KB) - added by SergeyBiryukov 3 years ago.
18857.2.diff (1.5 KB) - added by davilera 21 months ago.

Download all attachments as: .zip

Change History (50)

@apocalip
8 years ago

#1 @nacin
8 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
7 years ago

#23297 was marked as a duplicate.

#3 @SergeyBiryukov
7 years ago

  • Component changed from General to Plugins

#4 @SergeyBiryukov
7 years ago

  • Milestone changed from Awaiting Review to 3.6

#5 @brasofilo
7 years ago

  • Cc brasofilo@… added

#6 @scribu
7 years ago

  • Keywords has-patch added; dev-feedback removed

@scribu
7 years ago

corrrect format of plugin.diff

#7 @SergeyBiryukov
7 years ago

#21454 was marked as a duplicate.

#8 @SergeyBiryukov
6 years ago

#24274 was marked as a duplicate.

#9 @jrfoell
6 years ago

  • Cc justin@… added

#11 @SergeyBiryukov
6 years ago

Introduced in [2234].

#12 @toscho
6 years ago

  • Cc info@… added

#13 @ryan
6 years ago

  • Milestone changed from 3.6 to Future Release

#14 @nacin
6 years ago

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

#15 @nacin
6 years ago

  • Component changed from Admin APIs to Plugins

Sorry for the noise.

#16 @ocean90
6 years ago

#27732 was marked as a duplicate.

#17 @SergeyBiryukov
5 years ago

#30073 was marked as a duplicate.

#18 @nerrad
5 years ago

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

#19 @bobbingwide
4 years 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.


4 years ago

@SergeyBiryukov
3 years ago

#21 follow-up: @SergeyBiryukov
3 years 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 3 years ago by SergeyBiryukov (previous) (diff)

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


3 years ago

#23 in reply to: ↑ 21 @ocean90
3 years 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.


3 years ago

#25 @ocean90
3 years 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
3 years ago

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

#27 @ocean90
3 years ago

#38814 was marked as a duplicate.

#28 in reply to: ↑ description @sk.shaikat
2 years ago

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

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


2 years ago

#30 @davilera
21 months ago

#13266 was marked as a duplicate.

@davilera
21 months ago

#31 @davilera
21 months ago

  • Keywords has-unit-tests added; needs-unit-tests removed

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

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


20 months ago

#33 @swissspidy
19 months ago

  • Keywords needs-testing added

#34 @chriscct7
9 months ago

  • Keywords early needs-testing removed
  • Milestone changed from Future Release to 5.1
  • Owner changed from SergeyBiryukov to chriscct7
  • Status changed from accepted to assigned

#35 @pento
9 months ago

  • Keywords early added
  • Milestone changed from 5.1 to 5.2

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

#36 @chriscct7
9 months ago

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.

Last edited 9 months ago by chriscct7 (previous) (diff)

#37 @chriscct7
9 months ago

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.

#38 @jorbin
7 months ago

  • Milestone changed from 5.2 to Future Release

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

#39 @ocean90
3 months ago

#47805 was marked as a duplicate.

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


2 months ago

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


2 months ago

#42 @SergeyBiryukov
2 months ago

  • Milestone changed from Future Release to 5.3

#43 @davidbaumwald
4 weeks ago

  • Keywords early removed
  • Milestone changed from 5.3 to Future Release

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

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


12 days ago

#45 @garrett-eclipse
9 days ago

  • Keywords early added
Note: See TracTickets for help on using tickets.