Make WordPress Core

Opened 8 years ago

Closed 8 years ago

#36697 closed defect (bug) (fixed)

Combining the HTTP requests for obtaining the available nav menu items into a single request

Reported by: nguyencongtuan's profile nguyencongtuan Owned by: curdin's profile curdin
Milestone: 4.7 Priority: normal
Severity: normal Version: 4.3
Component: Customize Keywords: needs-testing has-patch has-unit-tests
Focuses: Cc:


Have many admin-ajax.php are called ( total depend on post types and taxonomy types are defined ) at same time when click on Menu from Customizer or Shift + click on menu from Preview Window that cause take many resource from server

Attachments (6)

36697.diff (6.1 KB) - added by curdin 8 years ago.
36697.2.diff (6.1 KB) - added by curdin 8 years ago.
Refreshed patch
36697.individually-deferred.diff (1.6 KB) - added by westonruter 8 years ago.
36697.3.diff (6.3 KB) - added by curdin 8 years ago.
Refreshed corrupted patch 36697.2.diff
36697.4.diff (6.1 KB) - added by westonruter 8 years ago.
36697.5.diff (9.5 KB) - added by curdin 8 years ago.

Download all attachments as: .zip

Change History (36)

This ticket was mentioned in Slack in #core-customize by nguyencongtuan. View the logs.

8 years ago

#2 @westonruter
8 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to Future Release

Agreed. The HTTP requests should be combined into a single request.

If anyone wants to own this we can milestone it for 4.6.

#3 @nguyencongtuan
8 years ago

Hi @westonruter ,

I will do it and submit the patch for it in the next month.

#4 @westonruter
8 years ago

  • Milestone changed from Future Release to 4.7

8 years ago

#5 @curdin
8 years ago

  • Keywords has-patch needs-testing dev-feedback added; needs-patch removed

Patch above uses most of the original code and simply puts iterators in where needed. Keen for someone to review and help me align with WordPress preferred syntax.

Also, open to suggestions for a cleaner way to manage the loading state of availableMenuItemContainer.

#6 @westonruter
8 years ago

  • Keywords needs-refresh added

@curdin thanks for the patch! Would you mind refreshing it? It's not applying anymore, probably due to [38436].

8 years ago

Refreshed patch

#7 @curdin
8 years ago

  • Keywords needs-refresh removed

@westonruter See above for the refreshed patch. I just got a whole lot better at fixing git merge conflicts :)

#8 @westonruter
8 years ago

I was just thinking about this some more today and I realized that I don't really understand why all of the available items need to be loaded up-front to being with. For one thing, the available items requests are being made when the menus panel is opened, but these could be wasted requests because they'll never be displayed if the user never opens the panel to chose from available nav menu items.

Secondly, then the user goes to “Add Items” the panel that opens shows all of the available item types in _collapsed_ accordions. None of the items are displayed there either. Would it not be much more efficient to just make the Ajax request to fetch the items of a given type when the accordion section is expanded? The spinner can be shown there initially to show that work is being done.

@celloexpressions thoughts?

(Another thought: If we do go ahead and combine all available nav menu items into a single request, there is a slight chance that there could be a server timeout if there are many custom post types and taxonomies. That being said, it's less likely to be problematic compared to hitting the server with X as many HTTP requests.)

#9 @westonruter
8 years ago

36697.individually-deferred.diff is an initial patch that loads the items for a given type the first time that the accordion section is opened. Seems like some refactoring should be done to move the data attributes to the top li so that a secondary query for .available-menu-items-list could be avoided.

#10 @celloexpressions
8 years ago

  • Version changed from 4.5 to 4.3

From the end user perspecitve, it's important that it seems like the content is already there when they open the panel. There can't/shouldn't be a delay between opening a section of available menu items and seeing at lest the initial items there to select from.

It might partially be because we fire so many requests at once, but even with the current setup I often have to wait for sections to finish loading by the time I get to opening them. Merging these into one request is probably the best approach to improving the user experience here. The vast majority of users won't care about content loading that they might not need/see; a corollary is the fact that most customizer objects are loaded (at least their data) when the customizer is opened even if they won't be used. They'll care if they have to wait for something to load, and anything that has to do an Ajax call has the potential to be quite slow and even to fail, even in my everyday experience with sub-par (for the US) connections.

In most cases the user will be opening the add-items panel when they go into menus, with the exceptions being changing locations, reordering, or deleting only, although those actions often also coincide with adding items. We're not talking about a lot of data here, so if we can start loading it in anticipation of the user needing it, the user is less likely to need to wait. The approach in 36697.2.diff seems like the best improvement we can make here.

#11 @westonruter
8 years ago

  • Keywords dev-feedback removed
  • Owner set to westonruter
  • Status changed from new to accepted

Fair enough, although the reason for why the customizer overall can be very slow to load is that too much is loaded up-front and not enough is lazy-loaded as needed (see #28580). In the case of nav menu items, loading all in one request should suffice as long as it doesn't block the initial page load, which it wouldn't here.

#12 @westonruter
8 years ago

@curdin I'm having trouble applying your patch. It seems the diff file is malformed:

patching file src/wp-admin/js/customize-nav-menus.js
patch: **** malformed patch at line 6:

Could you point me to the Git repo you are using or re-attach a patch that applies cleanly?

#13 @westonruter
8 years ago

@curdin Are you available to point me to your Git repo or a corrected patch file?

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

8 years ago

#15 @stevenkword
8 years ago

@westonruter believe this can be done in Beta for 4.7, but it looks like we are still waiting on a patch refresh.

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

8 years ago

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

8 years ago

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

8 years ago

#19 @celloexpressions
8 years ago

  • Keywords needs-refresh added

Looks like we're still pending an updated patch from @curdin here.

#20 @curdin
8 years ago

Hello folks, apologies I'm just back from 6 weeks of holidays - will refresh shortly.

#21 @westonruter
8 years ago

  • Owner changed from westonruter to curdin
  • Status changed from accepted to assigned

8 years ago

Refreshed corrupted patch 36697.2.diff

#22 @curdin
8 years ago

  • Keywords needs-refresh removed

@westonruter refreshed patch added. Apologies for the delay but I had an excellent holiday :)

#23 @westonruter
8 years ago

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

@curdin thanks for that.

I've opened a pull request for review and additional revisions. Here are the first few changes I've made:

A couple other outstanding issues that need to be fixed:

  • Pagination is broken when infinite scrolling through available nav menus of a given type. Fetching of new items for a given type needs to be updated to use the new request signature. So loadItems needs to work both with single type and multiple types.
  • PHP unit tests are broken.

Instead of modifying the existing Ajax request to require multiple item types, I think that perhaps loadItems should be smarter about whether its first argument is an array of itemType objects or a single itemType object. In the latter case, it can make the request with the object and type params as previously, and this would ensure backwards compatibility and should prevent the tests from breaking. But an array of itemType objects are passed, then this is when the Ajax request can be made with the an item_types array instead, and this is what would key in the new functionality to do the bulk request.

Is this something you can pick up? I'll attach my latest patch as well in case you don't want to collaborate on GitHub instead.

#24 @curdin
8 years ago

@westonruter - Yes, i can pick this up. Will ping you on slack if I get lost.

#25 @curdin
8 years ago

@westonruter Can I have your opinion please? I've been working on this for a bit today and wanted to check if backwards compatibility in this case is strictly necessary. The reason is that the code could be simplified if we dropped compatibility. Eg see patch with backwards compatibility vs patch without, where the ajax handler always expects an array of itemTypes

I'm not clear how widespread use for this function is outside core so looking for your expert opinion. I'll need to do some more work on whatever the preferred approach is.

Last edited 8 years ago by curdin (previous) (diff)

#26 @westonruter
8 years ago

@curdin backwards compatibility is not a big concern for this Ajax handler. But I don't think we have to break backwards compatibility do we? I mean, the existing ajax handler is taking a type and object params. Could not the request to fetch multiple items just omit these and pass an array of item types instead?

#27 @curdin
8 years ago

@westonruter sorry, I'm not sure I quite follow. I can certainly amend the loadItems to take either type and object, or an array passed into type. However the tests fail not because loadItems has been amended, but because in my initial patch the ajax handler was changed to receive an array.

My preferred approach would be the following:

  • 'loadItems' takes an array of itemType(s), if you prefer I can also leave the type and object params available
  • loadItems makes request to ajax handler using an array of itemTypes.
  • Ajax handler returns an array of itemTypes and their items. This breaks the current tests as they expect a set of items for the requested itemType only.
  • loadItems processes this array and injects data into DOM as required

Does this make sense? Give me a shout on Slack if not.

#28 @westonruter
8 years ago

@curdin I meant the PHP side of things. For the JS side of things we can maintain backwards compatibility by looking at the arguments passed to loadItems. If a second argument is passed, then we know that it's using the old signature of loadItems( type, object ). If only one argument is present, then we'd know that it's using the new signature.

Ajax handler returns an array of itemTypes and their items. This breaks the current tests as they expect a set of items for the requested itemType only.

Unit test could be amended to only pass in the specific expected item type, right?

In any case, let me know which PR has your preferred approach and once you're ready I'll do a review and commit. It's probably too late for beta2 but it could be part of beta3 next week, unless you can hand it over to me in the next couple hours.

#29 @curdin
8 years ago

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

#30 @westonruter
8 years ago

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

In 39137:

Customize: Combine Ajax requests for initial load of available nav menu items into a single request.

When there are many post types registered, prevent Ajax requests from piling up and slamming WordPress with concurrent requests.

Props curdin, westonruter.
Fixes #36697.

Note: See TracTickets for help on using tickets.