[wp-trac] [WordPress Trac] #18857: get_plugin_page_hookname uses menu_title to construct subpage load-hooks

[WordPress Trac]

#18857: get_plugin_page_hookname uses menu_title to construct subpage load-hooks
--------------------------------------------+-----------------------------
 Reporter:  apocalip                        |       Owner:  chriscct7
     Type:  defect (bug)                    |      Status:  assigned
 Priority:  normal                          |   Milestone:  5.2
Component:  Plugins                         |     Version:  3.1.4
 Severity:  normal                          |  Resolution:
 Keywords:  has-patch has-unit-tests early  |     Focuses:  administration
--------------------------------------------+-----------------------------

Comment (by 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.

-- 
Ticket URL: <https://core.trac.wordpress.org/ticket/18857#comment:37>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform