Make WordPress Core

Opened 14 years ago

Closed 13 years ago

#14415 closed defect (bug) (fixed)

Major fail of the pages widget in the nav-menu admin page when having a lot of pages

Reported by: dreadlox's profile DreadLox Owned by:
Milestone: 3.1 Priority: normal
Severity: normal Version: 3.0
Component: Menus Keywords: has-patch
Focuses: Cc:

Description

The title says it all. Having a hundred pages, under firefox: I can't go through the paging of the pages list widget in the nav-menu admin page. The error is: "script stack space quota is exhausted". Also search does not return any result.

Under chrome the search box doesn't work at all (no spinning indicator)

The result is that I CAN'T add some pages to my menu.

Attachments (3)

no-checkbox-menu-item-desc.14415.diff (1.2 KB) - added by filosofo 14 years ago.
omit-description-menu-item-checklist.14415.diff (2.7 KB) - added by filosofo 13 years ago.
14415.fix.multi.items.diff (2.2 KB) - added by duck_ 13 years ago.

Download all attachments as: .zip

Change History (20)

#1 @nacin
14 years ago

Version of PHP, version of Firefox, operating system?

#2 @filosofo
14 years ago

This probably has nothing to do with the total number of pages (each meta-box page of pages is limited to 50 per page) but more to do with the length of the content in the pages, which could cause the response to time out.

Try the patch no-checkbox-menu-item-desc.14415.diff to see if that solves the problem. There's no reason we need the page content hidden in the checkbox markup as far as I can see.

#3 @filosofo
14 years ago

  • Keywords reporter-feedback added
  • Milestone changed from Awaiting Review to 3.1
  • Version set to 3.0

#4 @scribu
14 years ago

  • Severity changed from blocker to normal

I was wondering why you would need descriptions for a simple checkbox list.

#5 follow-up: @scribu
13 years ago

Actually, the description is used to prefill the description field, when adding an item to the menu.

The description field is hidden by default; it can enabled from the screen options.

#6 in reply to: ↑ 5 @filosofo
13 years ago

Replying to scribu:

Actually, the description is used to prefill the description field, when adding an item to the menu.

The description field is hidden by default; it can enabled from the screen options.

There's no need to include it in the checkbox list though, since when you add an item the markup (including description field) is generated on the server-side anyways.

#7 @scribu
13 years ago

That's what I thought too, but then when I applied your patch, the description was blank.

The data is sent to the server to be saved as a new menu item, which is then generated.

It would be good to make it work as you suggest, but it won't be trivial to do.

#8 @filosofo
13 years ago

omit-description-menu-item-checklist.14415.diff works around it. Currently, it omits just the description property, but there's not reason we couldn't omit a couple of others.

#9 @filosofo
13 years ago

  • Keywords has-patch added; reporter-feedback removed

#10 @filosofo
13 years ago

Patch is still fresh and clean.

#11 @scribu
13 years ago

  • Resolution set to fixed
  • Status changed from new to closed

(In [16096]) Optimize menu item creation. Props filosofo. Fixes #14415

#12 @nacin
13 years ago

(In [16104]) Move code out of the conditional. props duck_, fixes #14415.

#13 @nacin
13 years ago

Extra bit snuck into that commit in response to a wp-hackers thread.

http://lists.automattic.com/pipermail/wp-hackers/2010-October/035614.html

Leaving it in.

#14 @duck_
13 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

This has also introduced a regression in the way multiple items of the same type are received. Previously you could add many Posts, for example, to a menu at once; now you only the first checked gets added.

The solution seems like it would be looping through the $_POSTmenu-item? array instead of just getting the first item with array_shift. Quick patch attached. Though this does change the $menu_data array received by wp_save_nav_menu_items to be non-negatively indexed (pre-[16096] had negative indexes), though I don't think this should matter since menu-item-db-id should still be empty but maybe needs some more thorough testing.

#15 @filosofo
13 years ago

Patch 14415.fix.multi.items.diff works in my tests.

#16 follow-up: @scribu
13 years ago

(In [16105]) Fix regression introduced by [16096]. Props duck_. See #14415

#17 in reply to: ↑ 16 @duck_
13 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

Replying to scribu:

(In [16105]) Fix regression introduced by [16096]. Props duck_. See #14415

Note: See TracTickets for help on using tickets.