WordPress.org

Make WordPress Core

Opened 2 years ago

Last modified 2 years ago

#36905 assigned enhancement

Add caching to get_page_by_title

Reported by: spacedmonkey Owned by: igmoweb
Milestone: Future Release Priority: normal
Severity: normal Version: 2.1
Component: Posts, Post Types Keywords: has-patch good-first-bug has-unit-tests
Focuses: performance Cc:

Description

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

Attachments (6)

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

Download all attachments as: .zip

Change History (14)

@spacedmonkey
2 years ago

#1 @spacedmonkey
2 years ago

  • Keywords has-patch needs-unit-tests added

Related: #36711

#2 @boonebgorges
2 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
2 years ago

Previously: #15805.

@igmoweb
2 years ago

get_page_by_title() using WP_Query

@igmoweb
2 years ago

Unit tests for 36905.2.patch

#4 @igmoweb
2 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
2 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
2 years ago

Changes + Unit tests

#6 @igmoweb
2 years ago

Done.

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

#7 @dd32
2 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
2 years ago

Some improvements + some more unit tests

@igmoweb
2 years ago

patch 4 was mixed up with another ticket changes

#8 @igmoweb
2 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.
Note: See TracTickets for help on using tickets.