WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 3 years ago

Last modified 3 years ago

#27042 closed defect (bug) (fixed)

Use of the_post() in _wp_ajax_menu_quick_search() is making in_the_loop = true

Reported by: ruud@… Owned by: ocean90
Milestone: 4.6 Priority: normal
Severity: normal Version: 3.0
Component: Menus Keywords: has-patch
Focuses: administration Cc:
PR Number:

Description

In /wp-admin/includes/nav-menu.php the menu quick search results are transfered to a json response by doing a search and looping over the results with a 'regular' have_posts(): the_post() construction.
As a side-effect the in_the_loop gets set to true in the_post() function.

This being an admin facing display of (custom)posts titles, I didn't suspect this behavior.

If this is indeed not meant to be, then I can think of 2 solutions:
1) Quick (and dirty?) add an if ( is_admin() ) check to the_post(), to prevent accidental setting of 'in_the_loop' while in the admin.
2) Don't use the_post() in _wp_admin_menu_quick_search().

For the latter I did some looking around in the admin for other similar situations and cooked up the attached patch.

While in this function I did some further code tidying.

Attachments (3)

nav-menu.php.patch (1.5 KB) - added by ruud@… 6 years ago.
nav-menu.php patch
nav-menu.php.37802.patch (573 bytes) - added by ruud@… 3 years ago.
refresh for patch
nav-menu.php.37802.WP_QUERY.patch (1.6 KB) - added by ruud@… 3 years ago.
patch using WP_QUERY instead of query_posts()

Download all attachments as: .zip

Change History (16)

@ruud@…
6 years ago

nav-menu.php patch

#1 @ruud@…
6 years ago

  • Keywords has-patch added

#2 @jeremyfelt
6 years ago

  • Keywords reporter-feedback added
  • Version changed from trunk to 3.0

It is interesting that we use the loop like that in the quick search. Is there a specific scenario where this becomes a problem?

Related, this was introduced in #12713

#3 @ruud@…
6 years ago

The scenario in which this became apparent (and a problem) is as follows:
We use a naming convention where a page/post title can have additional info enclosed with square brackets, e.g.: "Reception [senior department]"

We have multiple 'Reception' pages throughout our website for each department. This helps us greatly in some cases where page/post titles are shown in CMS dropdowns etc. the bracket designation helps identify the page you want (instead of random choosing 1 of perhaps 6 'Reception' pages)

To prevent showing this title to an end-user we use the 'the_title' filter with a 'in_the_loop' check. The filter then removes the square bracket part of the title.

However; due to the fact that the quick-search also sets in_the_loop to true.. the bracket notated meta info gets removed.. and we're back to square one with 6 similar 'Reception' pages to choose from :)

Last edited 6 years ago by ruud@… (previous) (diff)

#4 @ruud@…
6 years ago

  • Keywords reporter-feedback removed

#5 @ruud@…
6 years ago

  • Keywords dev-feedback added

#6 @chriscct7
4 years ago

  • Keywords needs-refresh added

#7 @ruud@…
3 years ago

  • Keywords needs-refresh removed

Patch refreshed to current trunk version (37802)

@ruud@…
3 years ago

refresh for patch

#8 @ruud@…
3 years ago

  • Keywords dev-feedback removed

Just had a quick discussion at WCEU contributor day 2016 with @jeremyfelt. The patch may be fixing this, however the usage of query_posts itself is also a bit odd. So instead of fixing this badly I'm going to propose another patch which removes the usage of the query_posts entirely

@ruud@…
3 years ago

patch using WP_QUERY instead of query_posts()

#9 @ruud@…
3 years ago

  • Keywords dev-feedback added

#10 @ruud@…
3 years ago

Using WP_Query instead of query_posts seems like a more performant way of obtaining this small list of pages. Also this solves the initial 'problem' of having a 'in_the_loop=true' value.
Somehow the 'in_the_loop=true' feels like a side-effect of the former/old method of getting posts for this specific purpose.

#11 @ocean90
3 years ago

  • Summary changed from Use of the_post() in _wp_admin_menu_quick_search() is making in_the_loop = true to Use of the_post() in _wp_ajax_menu_quick_search() is making in_the_loop = true

#12 @ocean90
3 years ago

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

In 37881:

Nav Menus: Use WP_Query for quick searches.

the_post() sets the $in_the_loop property to true which is unexpected in the admin if you're using filters which should only affect real loops.

Props ruud@joyo.
Fixes #27042.

#13 @ocean90
3 years ago

  • Keywords dev-feedback removed
  • Milestone changed from Awaiting Review to 4.6
Note: See TracTickets for help on using tickets.