menu_slug part of add_submenu_page ignores space chars

[utvikl]

There is a bug (and regression) with the menu_slug part of add_submenu_page and space characthers. If it's set like e.g. "Plugin Commander" the space gets removed in the URL that is added to the browser and therefore the correct method is not called when the user press the menu item.

I found this bug becuase it breaks the popular WPMU plugin "Plugin Commander".

[scribu]

Plese post the exact call to add_submenu_page().

[ocean90]

The plugin use this:

define('PC_HOME','wpmu-admin.php');
add_submenu_page(PC_HOME, __('Plugin Commander', 'plugin-commander'), __('Plugin Commander', 'plugin-commander'), 8,'Plugin Commander', 'pc_page');

And the link will be http://localhost/wp/wp-admin/ms-admin.php?page='''PluginCommander

It has worked in 2.9.

[ocean90]

Sry, the link is without '''. But the whitespace is missing.

[nacin]

Changing the space in the menu_slug parameter (the second to last) to %20 makes this work.

Yes, we broke it and that code did work in 2.9, but I'm not sure we should be inclined to fix it. The plugin author accounted for the need of %20 in define('PC_CMD_BASE',PC_HOME."?page=Plugin%20Commander"); yet failed to catch it here, probably because it worked. Spaces in URLs are never a good idea.

[wpmuguru]

I prefer the slug sans spaces and vote for wontfix.

[westi]

This was broken by this change:

https://core.trac.wordpress.org/changeset/13851/trunk/wp-admin/menu-header.php

Basically we now esc_url the slug and that strips out spaces.

[westi]

Which we could change the escaping to properly escape spaces here it still wouldn't make this plugin still work as it needs to register a hook name that will match when the url is parsed.

So I'm tempted to WONTFIX

[nacin]

From IRC:

<ocean90> nacin: He is using %20 in PC_CMD_BASE so he should use it there too. We could also inform the author.

Sending a note and closing as wontfix.

[omry]

Hi All,
I am the author of Plugin Commander.
while it's a minimal fix on my side to change this - it means all the users of the plugin will have to re-download it.
this is a pretty big hassle for something this trivial.
why not url-encode the slug name before putting it in a url in wp core?
this will replace the space with %20 and will maintain compatibility.

[westi]

url-encoding the slug name won't help as it doesn't match elsewhere when the slugs are matched for selecting the page to display.

It is better for the plugin to correctly register.

The fact that this used to work could be classed as a bug.

[utvikl]

It would be interesting to do some grepping in a repository of plugins to see how many plugins is affected by this. The problem is that it's a regression and that it fails ungratefully and it will cause trouble for end users.

The menu_slug is also not very well defined in the documentation so this should be improved.

Other than that I agree that this should be fixed in the plugin, but I would consider grepping some plugin repository or at least give an grateful error to the end user. I guess there are even more chars than \s that could cause trouble. Chars like %, &, ? and / should not be in the menu_slug aswell?