WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 5 years ago

#11092 closed defect (bug) (wontfix)

get_body_class() needless messes with global variables

Reported by: filosofo Owned by: ryan
Milestone: Priority: normal
Severity: normal Version:
Component: Template Keywords: get_body_class has-patch tested
Focuses: Cc:

Description

get_body_class() calls setup_postdata() without reason.

Also, it makes a direct database call, when using the API would be better:

  • Avoid including drafts and private posts as part of its condition
  • Avoid querying the database directly is a good idea wherever possible.
  • Caching benefits

Patch fixes.

Attachments (1)

unnecessary_global_tampering.11092.diff (1.9 KB) - added by filosofo 6 years ago.

Download all attachments as: .zip

Change History (7)

comment:1 @ryan6 years ago

Related [10877]

comment:2 @Denis-de-Bernardy6 years ago

I'm suspicious of the suggested api call. the get_var() call could arguably be improved (to exclude drafts, etc.), but it's probably faster than any api call we can think of, since it's using an index to fetch a primary key (i.e. it shouldn't even read the db tables).

comment:3 follow-up: @filosofo6 years ago

Can you substantiate your suspicions? Here's the actual database query for a page using my patch:

SELECT * FROM wp_posts  WHERE (post_type = 'page' AND post_status = 'publish')    AND post_parent = 37   ORDER BY post_title ASC LIMIT 0,1

and here it is without the patch (current behavior):

SELECT ID FROM wp_posts WHERE post_parent = 37 AND post_type = 'page' LIMIT 1

Both claim to take 0.00 seconds with the query cache off.


it's probably faster than any api call we can think of

How about an API call that doesn't even have to connect to the database at all? That's a possibility which the object cache.

get_var() call could arguably be improved (to exclude drafts, etc.),

We shouldn't be trying to figure out this logic in multiple places (DRY). It's almost guaranteed to produce vexing bugs in the future, for page statuses that we forget about.

comment:4 in reply to: ↑ 3 @Denis-de-Bernardy6 years ago

Replying to filosofo:

Can you substantiate your suspicions?

Sure...

Here's the actual database query for a page using my patch:

SELECT * FROM wp_posts  WHERE (post_type = 'page' AND post_status = 'publish')    AND post_parent = 37   ORDER BY post_title ASC LIMIT 0,1

and here it is without the patch (current behavior):

SELECT ID FROM wp_posts WHERE post_parent = 37 AND post_type = 'page' LIMIT 1

Both claim to take 0.00 seconds with the query cache off.

these two queries potentially have very different behaviors in reality.

had there been an index on (post_parent, post_type) the second one would be proceed as follows:

  1. open index on (post_parent, post_type) if it's not loaded in the memory already
  2. scan index for post_type = page with post_parent = 37 (fast)
  3. return the first ID found (fast)

since we're missing that index, it'll actually load rows one by one and return the ID of the first row it finds that works (making it much slower than it should be):

  1. open the relevant index (likely post_parent in your example)
  2. scan index for rows that work (fast)
  3. until an ID is found, open the needed disc page if needed (slow) and load the field we're missing (post_type) for that row (fast)
  4. filter rows that don't work (fast)
  5. return the first ID that works (fast)

by contrast, the query using the API does:

  1. open whichever index the query engine decides is correct (probably post_parent in your example) if not loaded already
  2. scan index for IDs that potentially work (fast)
  3. for each ID, open the needed disc page if needed (slow) and load the fields we're missing (post_type, post_title) for that row (fast)
  4. filter rows that don't work (fast)
  5. filter if it's not the first row (order by post_title limit 1) (slow if the result set is large, and I'm probably wrong to make the assumption that MySQL does a top-1 search on the fly instead of a quicksort on the full set)
  6. load the entire row (slow)
  7. return the entire row (slow if post_content is large)

I hope my suspicions make more sense...

comment:5 @ryan5 years ago

  • Milestone changed from 2.9 to 3.0

comment:6 @filosofo5 years ago

  • Milestone 3.0 deleted
  • Resolution set to wontfix
  • Status changed from new to closed
  • Version 2.9 deleted

This ticket got sidelined into punt limbo by a controversial line in the patch which wasn't even the main point of the patch.

I've taken out the controversial line and re-focused the issue in #11439.

Note: See TracTickets for help on using tickets.