Make WordPress Core

Opened 2 years ago

Last modified 23 months ago

#57211 assigned enhancement

Split up `WP_Query::get_posts()`

Reported by: costdev's profile costdev Owned by: costdev's profile costdev
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Query Keywords: has-patch dev-feedback
Focuses: Cc:

Description

WP_Query::get_posts() is a huge method at ~1650 lines. This makes it quite hard to follow and maintain.

By splitting it up:

  • It's easier to follow.
  • It's easier to maintain.
  • It's easier to write tests.
  • Our scroll wheels will last a little longer.

As this is possibly the most critical method in Core, the aim of this ticket is not to refactor the logic, but just to abstract functionality. Changes to logic and such could be done later, in more maintainable code.

This should significantly reduce the chance of introducing regressions, and hopefully mean that committing such a change can be done in the near future. All new methods should have private visibility.

Change History (3)

This ticket was mentioned in PR #3667 on WordPress/wordpress-develop by @costdev.


2 years ago
#1

WP_Query::get_posts() is a huge method at ~1650 lines. This makes the method hard to follow and maintain.

This PR is an attempt to split this method into smaller pieces by abstracting some functionality to new private methods.

At the moment:

  • WP_Query::get_posts() is reduced from ~1650 lines to ~920 lines - roughly a reduction of 45%.
  • 30 new private methods have been introduced.

Please note:

  • This is just an experiment for now. If reviewers think this is worth pursuing, I'll attach this PR to a ticket for consideration.
  • At this stage, the intention is not to refactor logic, but to abstract to make future refactoring easier.
  • Naming things is hard.

Props to @peterwilsoncc for skilfully convincing me that I should be the one to try this, and to @mukeshpanchal for a review of the initial abstractions.

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

@costdev commented on PR #3667:


2 years ago
#2

I ran a coverage report to see what line coverage is like with the existing tests. Bear in mind that @covers annotations aren't complete in the test suite and such, but this shows the code is executing and in terms of what the test methods are specifically testing, there are no failures.

get_posts() is the only original method. From parse_meta_query() to apply_filters_for_caching_plugins() are the 30 new private methods introduced in this PR. The tests haven't been modified at all.

https://i0.wp.com/user-images.githubusercontent.com/79332690/204143909-8142e0f4-4f3f-4a8f-a3fa-d5171bfceb9e.png

#3 @costdev
2 years ago

  • Keywords dev-feedback added

@audrasjb @SergeyBiryukov I've tried to keep this as minimal as possible. It's a partial splitting up, and I haven't tried to refactor any logic in the process. Despite this, I understand that this kind of change would come with concerns about getting it right. That said, I think the sooner we can get this reviewed, the better, and if there's specific testing you have in mind that you'd like to be done before commit, then we can get the ball rolling on that.

Are either/both of you available to review the PR? And are there any other committers you'd like to take a look at it?

Last edited 23 months ago by costdev (previous) (diff)
Note: See TracTickets for help on using tickets.