Make WordPress Core

Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#13722 closed defect (bug) (wontfix)

menu_slug part of add_submenu_page ignores space chars

Reported by: utvikl Owned by: westi
Milestone: Priority: normal
Severity: normal Version: 3.0
Component: Plugins Keywords: close
Focuses: Cc:


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

Change History (16)

#1 @utvikl
8 years ago

  • Cc utvikl added

#2 @scribu
8 years ago

Plese post the exact call to add_submenu_page().

#3 @scribu
8 years ago

  • Keywords reporter-feedback added

#4 @ocean90
8 years ago

  • Keywords reporter-feedback removed
  • Milestone changed from Unassigned to 3.0

The plugin use this:

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.

#5 @ocean90
8 years ago

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

#6 @nacin
8 years ago

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.

#7 @nacin
8 years ago

  • Owner set to westi
  • Status changed from new to assigned

#8 @nacin
8 years ago

  • Component changed from General to Plugins

#9 @wpmuguru
8 years ago

I prefer the slug sans spaces and vote for wontfix.

#10 @westi
8 years ago

This was broken by this change:


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

#11 @westi
8 years ago

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

#12 @nacin
8 years ago

  • Keywords close added

#13 @nacin
8 years ago

  • Milestone 3.0 deleted
  • Resolution set to wontfix
  • Status changed from assigned to closed

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.

#14 @omry
8 years ago

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.

#15 @westi
8 years ago

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.

#16 @utvikl
8 years ago

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?

Note: See TracTickets for help on using tickets.