Make WordPress Core

Opened 8 years ago

Closed 19 months ago

Last modified 19 months ago

#36905 closed enhancement (fixed)

Add caching to get_page_by_title

Reported by: spacedmonkey's profile spacedmonkey Owned by: spacedmonkey's profile spacedmonkey
Milestone: 6.1 Priority: normal
Severity: normal Version: 2.1
Component: Posts, Post Types Keywords: has-patch good-first-bug has-unit-tests dev-feedback commit has-dev-note
Focuses: performance Cc:

Description

Add caching to get_page_by_title so it doesn't call the posts table.

Attachments (9)

36905.patch (1.2 KB) - added by spacedmonkey 8 years ago.
36905.2.patch (1.4 KB) - added by igmoweb 8 years ago.
get_page_by_title() using WP_Query
36905.2.tests.patch (4.1 KB) - added by igmoweb 8 years ago.
Unit tests for 36905.2.patch
36905.3.patch (5.5 KB) - added by igmoweb 8 years ago.
Changes + Unit tests
36905.4.patch (8.1 KB) - added by igmoweb 8 years ago.
Some improvements + some more unit tests
36905.5.patch (5.5 KB) - added by igmoweb 8 years ago.
patch 4 was mixed up with another ticket changes
36905.6.patch (5.7 KB) - added by pcfreak30 5 years ago.
patch refresh
36905.6.2.patch (5.7 KB) - added by pcfreak30 5 years ago.
patch refresh
36905.7.patch (6.1 KB) - added by spacedmonkey 5 years ago.

Download all attachments as: .zip

Change History (41)

@spacedmonkey
8 years ago

#1 @spacedmonkey
8 years ago

  • Keywords has-patch needs-unit-tests added

Related: #36711

#2 @boonebgorges
8 years ago

  • Keywords good-first-bug added
  • Milestone changed from Awaiting Review to Future Release

Change seems OK, but get_page_by_title() has no automated test coverage at all. We'll need basic tests to introduce this kind of change.

#3 @boonebgorges
8 years ago

Previously: #15805.

@igmoweb
8 years ago

get_page_by_title() using WP_Query

@igmoweb
8 years ago

Unit tests for 36905.2.patch

#4 @igmoweb
8 years ago

I propose to let WP_Query handle this.

I'd also propose to deprecate this function. Is not used by Core so plugins could use their own functions instead. Querying by title is not a very good idea so we would be discouraging developers to use this function (maybe just a little but more than nothing). @scribu said the same in here: https://core.trac.wordpress.org/ticket/20350#comment:1 I don't know if she would think the same today.

Anyway, here are my changes: Unit tests + changes inside the function (not deprecated though) so now it uses WP_Query instead.

#5 @DrewAPicture
8 years ago

  • Keywords has-unit-tests added; needs-unit-tests removed
  • Owner set to igmoweb
  • Status changed from new to assigned

Assigning to mark the good-first-bug as "claimed".

@igmoweb Can you please combine 36905.2.patch and 36905.2.tests.patch into a single patch?

@igmoweb
8 years ago

Changes + Unit tests

#6 @igmoweb
8 years ago

Done.

Do we now upload changes + tests in the same file? I thought it was the other way.

#7 @dd32
8 years ago

Do we now upload changes + tests in the same file? I thought it was the other way.

We used to do it separately as they were in two different repo's, they're now combined so doing it in one patch is easier to keep track of everything (especially when things start getting into multiple iterations)

@igmoweb
8 years ago

Some improvements + some more unit tests

@igmoweb
8 years ago

patch 4 was mixed up with another ticket changes

#8 @igmoweb
8 years ago

36905.5.patch includes some improvements to the function:

  • orderby is set to 'none' so it preserves the same order than the original function
  • post_status is now an array with 'any' + all existent statuses even those that are excluded from search. This way, the query has less conditionals in WHERE clause and post_status field is not even filtered, like the original function.
  • I added tests to check that the function query every single post status.

@pcfreak30
5 years ago

patch refresh

@pcfreak30
5 years ago

patch refresh

#9 @pcfreak30
5 years ago

Added a refresh based on current trunk. A duplicate copy was added due to internet issues :P. Refresh has not been tested, but mainly fixed to apply properly, and explicitly return null.

#10 @spacedmonkey
5 years ago

@igmoweb @pcfreak30 Looking at your patches, I have one simple question, how does this fix the issue? Using WP_Query is nice, but WP_Query isn't have any query caching out of the box. Even if are you using advanced-post-cache or enhanced-post-cache neither of which, cache queries with 'fields' => 'ids' set.

See that #36711 is basically the same code and @boonebgorges merged that. We should go back to my original patch. This will likely need a refresh after all coding standards changes.

#11 @pcfreak30
5 years ago

@spacedmonkey your patch looks nice however wp_query does cache with 'cache_results' if wp_using_ext_object_cache() is true, so the extra cache check in your version is unneeded. Even with it, it only helps if doing memory caching, which is up for debate if the extra complexity matters vs using core wp_query.

Also, I generally feel direct DB access for stuff like this should be avoided overall. We have a query system, it should be used if possible.

#12 @spacedmonkey
5 years ago

@pcfreak30 Sadly, you are incorrect, with the cache_results flag set, all that does is prime post caches and the query it's self is not cached. See this and the related ticket #22176 .

However, I do agree, we should be using WP_Query, as it is clearer that way. I will update the patch to fix the caching in with the query call changes.

#13 @desrosj
5 years ago

#46439 was marked as a duplicate.

#14 @spacedmonkey
2 years ago

@igmoweb Are you willing to work on this patch still? I am willing to pick it up otherwise.

#15 @igmoweb
2 years ago

@spacedmonkey Feel free to take it again, thanks :)

This ticket was mentioned in PR #2384 on WordPress/wordpress-develop by pbearne.


2 years ago
#16

refresh and tests
Tests not working

Trac ticket: https://core.trac.wordpress.org/ticket/36905

#18 @spacedmonkey
19 months ago

Now the WP_Query is cached, let's just use WP_Query in this function.

spacedmonkey commented on PR #3236:


19 months ago
#20

CC @pbearne.

#21 @spacedmonkey
19 months ago

  • Milestone changed from Future Release to 6.1
  • Owner changed from igmoweb to spacedmonkey

#22 @spacedmonkey
19 months ago

  • Keywords dev-feedback added

#23 @peterwilsoncc
19 months ago

  • Keywords commit added

Commit ready as of 88bee5302227b0a27e8f2a0d8b2691b509c065a2 on the linked pull request.

#24 @peterwilsoncc
19 months ago

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

In 54234:

Posts, Post types: Cache get_page_by_title().

Convert get_page_by_title() to a WP_Query wrapper to take advantage of the object caching built in to the latter.

Add dedicated unit tests for the function get_page_by_title().

Props spacedmonkey, boonebgorges, igmoweb, pcfreak30, pbearne.
Fixes #36905.

#27 @peterwilsoncc
19 months ago

In 54242:

Posts, Post types: Coding standards fixes following [54234].

Props costdev.
See #36905.

peterwilsoncc commented on PR #3285:


19 months ago
#28

Merged https://core.trac.wordpress.org/changeset/54242 / f450acf7afdce2859d5ede3656626ca5538b592b

#29 @peterwilsoncc
19 months ago

In 54271:

Posts, Post Types: Fix WP_Query parameter used by get_page_by_title().

Fixes the call to WP_Query within get_page_by_title() by using the correct title parameter, title.

Modify the orderby parameter to prioritize the oldest published date over the smallest post ID. This ensures the behaviour matches that of the previous version of get_page_by_title().

The tests have been modified to include a populated post table to ensure the posts returned are matched by design rather than coincidence.

Follow up to [54234].

Props dd32, timothyblynjacobs, peterwilsoncc.
Fixes #56609.
See #36905.

#30 @spacedmonkey
19 months ago

  • Keywords needs-dev-note added

#31 @spacedmonkey
19 months ago

Made a start of dev notes here.

Note: See TracTickets for help on using tickets.