WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

#18822 closed defect (bug) (fixed)

get_permalink() produces an add. DB request without $post->filter

Reported by: F J Kaiser Owned by: ryan
Milestone: 3.3 Priority: normal
Severity: normal Version: 3.2.1
Component: Performance Keywords: has-patch
Focuses: Cc:

Description

When used in a Query-Loop, get_permalink() makes an additional query $post = &get_post($id); to produce a valid link to the post (in example: archive pages). This means that you'll get one additional query per post-permalink on your archive page, which can pretty fast sum up.

When looking at link-template.php line 95, you can see the if/else statement that avoids throwing a complete $post object into the function to produce a permalink without an additional query.

The exact line that prevents using the post object:

if ( is_object($id) && isset($id->filter) && 'sample' == $id->filter ) {

So far the only possible solutions that can prevent that behaviour are shown in this Q on WordPress StackExchange - see answer by Scribu & Edit 3 by myself.

I haven't tested this for a patch, but maybe replacing the line with

if ( is_object($id) XOR (isset($id->filter) && 'sample' === $id->filter) ) {

would be enough, to make both the test cases/samples work as well as to prevent the additional query.

As adding a permalink to a post on archive pages is pretty common, I'd say this could be close to a major perfomance increase for WordPress Loops.

Attachments (4)

18822.diff (477 bytes) - added by scribu 3 years ago.
18822.alt.diff (833 bytes) - added by scribu 3 years ago.
18822.alt.2.diff (819 bytes) - added by scribu 3 years ago.
18822.2.diff (1.0 KB) - added by scribu 3 years ago.

Download all attachments as: .zip

Change History (17)

comment:1 solarissmoke3 years ago

  • Cc solaris.smoke@… added

comment:2 follow-up: duck_3 years ago

For the reasoning behind this code see #8526 and [10213]. "Don't cache filtered post objects. Set filter when getting sample permalink."

Check get_post() and you'll see that that function doesn't always produce an additional query. For example:

$posts = get_posts();
foreach ( $posts as $post )
    var_dump( get_permalink( $post->ID ) );

will not cause extra queries as all those posts are cached. You can also pass the whole $post object to get_permalink for no extra queries as get_post() accepts an object. In fact get_post() will only perform a query if you pass an object when the object has the filter property set and that post ID isn't already cached.

It seems to me that you must be doing something like direct DB queries to get post IDs and then using those to get permalinks. However you're doing it you're not getting posts added to the cache when retrieving them to pass to get_permalink().

comment:3 scribu3 years ago

  • Component changed from Query to Performance

comment:4 F J Kaiser3 years ago

  • Cc 24-7@… added

comment:5 in reply to: ↑ 2 F J Kaiser3 years ago

Replying to duck_:

In fact get_post() will only perform a query if you pass an object when the object has the filter property set and that post ID isn't already cached.

If you look closer at the code (in core & on wpse) you'll see that there's the filter name 'sample' required. Default is 'raw'. So, until you don't set the filter to 'sample' manually, you'll get an additional query and passing the object to the function won't help anything. Tested this in various cases - see also @scribu work around.

Last edited 3 years ago by F J Kaiser (previous) (diff)

comment:6 scribu3 years ago

It seems to me that you must be doing something like direct DB queries

Not necessarily:

$posts = get_posts( array(
  'cache_results' => false,
) )

Each post object will have [filter] => raw.

But get_post() only checks empty( $post->filter ):

} elseif ( is_object($post) && empty($post->filter) ) {

It should check if the filter is 'raw' before going off and doing another query:

} elseif ( is_object($post) && ( empty($post->filter) || 'raw' == $post->filter ) ) {

comment:7 scribu3 years ago

Or rather, it should have different handling for the 'raw' filter.

scribu3 years ago

comment:8 scribu3 years ago

  • Keywords has-patch added; needs-patch removed

comment:9 duck_3 years ago

Replying to F J Kaiser:

If you look closer at the code (in core & on wpse) you'll see that there's the filter name 'sample' required.

I think you misunderstood what I was saying or I misread the original ticket description. I was pointing out that a call to get_post() doesn't necessarily incur an additional database query as it will try to use the cache when possible, versus "additional query $post = &get_post($id) ... This means that you'll get one additional query" from the description.

Replying to scribu:

Not necessarily

I was trying to be vague in my guesswork on the methodology as I'm aware there are other methods this can occur. My mistake as that wasn't clear enough.

It should check if the filter is 'raw' before going off and [potentially] doing another query

That seems to make sense.

scribu3 years ago

comment:10 scribu3 years ago

  • Severity changed from major to normal
  • Type changed from enhancement to defect (bug)

Actually, I think the bug is that WP_Query runs sanitize_post() even when 'cache_results' is false.

See 18822.alt.diff.

scribu3 years ago

comment:11 scribu3 years ago

18822.alt.2.diff uses a foreach loop, which is faster. No idea why a for loop was used there.

scribu3 years ago

comment:12 scribu3 years ago

  • Milestone changed from Awaiting Review to 3.3

Talked to ryan on IRC, where I learned (heavily paraphrased) that duh! of course posts should always be sanitized. The comment in the code was just misleading.

Also, 18822.diff looks like a good idea. So, 18822.2.diff:

  • special handling for when 'filter' = 'raw' in get_post()
  • use foreach + replace comment in WP_Query.

comment:13 ryan3 years ago

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

In [18846]:

Avoid refetching a post in get_post() when passed a post object that has already been raw sanitized. Clean up sanitize loop. Props scribu. fixes #18822

Note: See TracTickets for help on using tickets.