Make WordPress Core

Opened 11 years ago

Closed 10 years ago

Last modified 10 years ago

#24684 closed enhancement (maybelater)

posts_clauses Query filter should have array as argument

Reported by: f-j-kaiser's profile F J Kaiser Owned by:
Milestone: Priority: normal
Severity: normal Version: 3.5.2
Component: Query Keywords: needs-patch
Focuses: Cc:

Description

Proposal to make it easier to modify the main, core-built query string.

In its current state, the posts_clauses filter and its siblings (posts_where, etc.) all have a string to modify. This can be really tough as one has to

  • use a Regex
  • consider every possible (edge) case

just to replace, add or remove parts of the SQL query. The reason for this is that get_posts() concatenates to the strings.

What I'd like to propose is exchanging the $where .= 'foobar'; string concatenations with an array core would add to: $where[] = 'foobar';. This would make it much easier to search through the different parts and replace/add/remove parts of it before the final query string is built.

I'd like to propose this as two step process:

  1. I'd add a patch that replaces the string concatenations with the array building process. The current arguments would stay in place, but the arrays would be implode()d for the filter calls. Then we could add another, temporary filter that's named posts_clauses_arrays, that would hold all those arrays as subarrays of $pieces.
  2. If every developer is familiar with this, we could change the arguments for the sibling filters and possibly remove the posts_clauses-filter in favor of the posts_clauses_array-filter.

Change History (11)

#1 @F J Kaiser
11 years ago

  • Cc 24-7@… added

#2 @birgire
11 years ago

  • Cc birgire added

#3 follow-up: @wonderboymusic
11 years ago

  • Keywords needs-patch close added

This would just end up being a 3rd argument to the post_clauses filter (back compat) and would be a major rewrite of get_posts() - unless a patch materializes that nails it, I don't see this picking up any steam

#4 in reply to: ↑ 3 @F J Kaiser
11 years ago

  • Keywords close removed

Replying to wonderboymusic:

This would just end up being a 3rd argument to the post_clauses filter (back compat) and would be a major rewrite of get_posts() - unless a patch materializes that nails it, I don't see this picking up any steam

You misread something: There would be no 3rd argument and no major rewrite. Actually the patch would just replace all the string concatenating with array pushing. All the filters and the vars would then just get converted from Array > String using a simple join()/implode() before they'd get called.

The new filter would run after the current filters so nothing would break and everything would be backwards compatible. Plugins could slowly move towards using the newer (easier and faster) filter.

#5 follow-up: @wonderboymusic
11 years ago

no major rewrite
Actually the patch would just replace all the string concatenating with array pushing

What isn't major about that?

#6 in reply to: ↑ 5 @TJNowell
11 years ago

Replying to wonderboymusic:

no major rewrite
Actually the patch would just replace all the string concatenating with array pushing

What isn't major about that?

I'd have to agree with Kaiser, but I would say get_posts is in desperate need of a major refactor. Or at least breaking up into separate methods

Infact, I know with certainty it is required, most computer scientists will advise that a method should have an NPath complexity of less than 200, and get_posts has an NPath complexity of:

1,435,733,941,397,422,709,124,940,625,188,500,371,668,992,000,000

or 1x1049, which is 25 orders of magnitude greater than the number of stars in the visible universe

When a method needs more unit tests to verify it works than there are stars in the universe and grains of sand on all the beaches in the solar system, I can surmise that swapping out some string concatenation for some array insertions is trivial in comparison.

That stupendous number could easily be brought down very quickly, and the changes necessary will go a long way towards making that method more maintainable. Perhaps Kaiser has a patch ready I'm unaware of, but anything that modifies get_posts is likely to be a good thing, after all, that number can't get much higher.

For those watching who are unaware http://www.codingswag.com/2013/05/cyclomatic-and-npath-complexity-explained/

#7 @wonderboymusic
11 years ago

Bros - post a patch and we can take it from there

#8 @TJNowell
11 years ago

Out of curiosity, if each unit test is 1.5kb and we use 1TB samsung drives at .4kg each, the unit tests needed would weigh 80 Undecillion KG ( 38 zeros ).

Or 4 solar masses. The lightest black hole found was 3.8 solar masses, and I hear disk platters are dense

Last edited 11 years ago by TJNowell (previous) (diff)

#9 @wonderboymusic
10 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to maybelater
  • Status changed from new to closed

There is a lot to do here to even come up with a proof of concept that is backwards-compatible. No one has produced a patch in the past 8 months.

#10 @helen
10 years ago

FWIW, there's a Trac comment with POC from MikeSchinkel proposing this at a larger scale, which isn't unreasonable, just seemingly a significant undertaking. I can certainly see this being desirable, and think we can probably reopen this as some point with focus. No need to go xkcd-what-if on unit tests right now. :)

#11 @TJNowell
10 years ago

Just pointing out that we should welcome a refactor not avoid it, the only way is up from that concern.

Note: See TracTickets for help on using tickets.