Make WordPress Core

Opened 2 years ago

Closed 2 years ago

#55699 closed defect (bug) (fixed)

posts_join_request filter is broken

Reported by: 5um17's profile 5um17 Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 6.0 Priority: normal
Severity: normal Version: 6.0
Component: Query Keywords: has-patch has-unit-tests commit dev-reviewed
Focuses: Cc:

Description (last modified by SergeyBiryukov)

One of the users reported an error with WP 6.0-RC1.

On checking, I found posts_join_request filter is stopped working since latest changes. First [52974] then [53175] for #54728.

I think this is an oversight, we missed adding compact() to

<?php
$clauses = (array) apply_filters_ref_array( 'posts_clauses_request', array( $clauses, &$this ) );

Attachments (5)

55699.patch (1.7 KB) - added by 5um17 2 years ago.
55699.2.diff (1.5 KB) - added by SergeyBiryukov 2 years ago.
55699.3.diff (2.7 KB) - added by SergeyBiryukov 2 years ago.
55699.4.diff (3.4 KB) - added by SergeyBiryukov 2 years ago.
55699.5.diff (6.1 KB) - added by SergeyBiryukov 2 years ago.

Download all attachments as: .zip

Change History (24)

@5um17
2 years ago

#1 @johnbillion
2 years ago

  • Milestone changed from Awaiting Review to 6.0

Thanks for the report, moving to 6.0 for visibility.

#2 @5um17
2 years ago

  • Keywords has-patch added

Thanks for the feedback @johnbillion!

#3 @SergeyBiryukov
2 years ago

  • Description modified (diff)

#4 @SergeyBiryukov
2 years ago

Good catch, thanks!

#5 @SergeyBiryukov
2 years ago

I don't think we should rename the variable back to $pieces though, as using a consistent name was the point of [52974]. Just restoring the compact call, same as for the posts_clauses filter in [53175] would fix the issue.

#6 @SergeyBiryukov
2 years ago

  • Keywords has-unit-tests added

55699.3.diff includes a unit test.

#7 @5um17
2 years ago

Replying to SergeyBiryukov:

I don't think we should rename the variable back to $pieces though, as using a consistent name was the point of [52974]. Just restoring the compact call, same as for the posts_clauses filter in [53175] would fix the issue.

Thanks @SergeyBiryukov! I renamed the variable because $clauses is being used two times in the code. And its value is changed after posts_clauses filter.

I can see in 55699.2.diff you have reinitialized the variable which is good too. Thanks!

#8 @SergeyBiryukov
2 years ago

55699.4.diff also includes a unit test for [53175].

It is worth noting that posts_clauses and posts_clauses_request were the only filters affected here. [53175] fixed the former, but missed the latter.

Other *_clauses filters adjusted in [52974] and [53175] are unaffected because there are no hooks between the $clauses array definition and the filter itself, so the logic for them has not changed.

#9 @SergeyBiryukov
2 years ago

  • Owner set to SergeyBiryukov
  • Resolution set to fixed
  • Status changed from new to closed

In 53370:

Query: Restore late compact() call for the posts_clauses_request filter.

This addresses a backward compatibility break where posts_join_request and other filters were applied, but their results were subsequently discarded and earlier values were used instead.

Follow-up to [52974], [53175].

Props 5um17, johnbillion, SergeyBiryukov.
Fixes #55699.

#10 @SergeyBiryukov
2 years ago

  • Keywords commit dev-feedback added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for backporting to the 6.0 branch after a second committer's review.

This ticket was mentioned in Slack in #core by chaion07. View the logs.


2 years ago

#12 follow-up: @peterwilsoncc
2 years ago

  • Keywords changes-requested added; commit dev-feedback removed

I'm concerned about [53370] and it needs additional work before it's ready to be merged in to the 6.0 branch.

In the new code, $clauses undergoes four main definitions:

  1. The name of the MySQL sub-clauses
  2. The value of the MySQL sub-clauses
  3. The name of the MySQL sub-clauses
  4. The value of the MySQL sub-clauses

It took me quite a long time to figure out what was happening as a result.

As a defensive coding measure, two variables need to be used one for the names, one for the values of the clauses.

I see in the original patch the suggestion was pieces and clauses. I think this is preferable to what is in trunk at the moment, despite the inconsistency with other query classes.

#13 in reply to: ↑ 12 @SergeyBiryukov
2 years ago

Replying to peterwilsoncc:

As a defensive coding measure, two variables need to be used one for the names, one for the values of the clauses.

I see in the original patch the suggestion was pieces and clauses. I think this is preferable to what is in trunk at the moment, despite the inconsistency with other query classes.

Sounds good, though if we're going to restore the $pieces name, I would ideally prefer to do that consistently for all query classes, essentially reverting [52974], as I don't see a reason for them to be different, and appreciate consistency where possible. In hindsight, the reason for [52974] was that $clauses seemed like a better name than $pieces for the *_clauses filter parameters, and I think that part can remain.

Does 55699.5.diff address the concern?

#14 @peterwilsoncc
2 years ago

I agree the consistency is a good idea.

For the purposes of the 6.0 back port , are you able to limit the commit to the buggy class only and do the renaming for consistency in a follow up commit.

Based on a visual review the patch and approach looks sound.

#15 @SergeyBiryukov
2 years ago

In 53375:

Coding Standards: Restore the $pieces variable in WP_Query::get_posts().

This is a defensive coding measure that aims to reduce confusion. With this change, $pieces is explicitly used for the names, and $clauses for the values of the clauses.

Follow-up to [52974], [53175], [53370].

Props peterwilsoncc.
See #55699.

#16 @SergeyBiryukov
2 years ago

In 53376:

Coding Standards: Restore the $pieces variable for SQL clauses in query classes.

This is a defensive coding measure that aims to reduce confusion. With this change, $pieces is explicitly used for the names, and $clauses for the values of the clauses.

Follow-up to [52974], [53175], [53370], [53375].

Props peterwilsoncc.
See #55699.

#17 @SergeyBiryukov
2 years ago

  • Keywords commit dev-feedback added; changes-requested removed

Marking [53375] for backporting to the 6.0 branch after a second committer's review.

[53376] can stay in trunk only, per comment:14.

#18 @hellofromTonya
2 years ago

  • Keywords dev-reviewed added; dev-feedback removed

Marking [53375] dev-reviewed for 6.0-branch backporting per #comment:14.

#19 @SergeyBiryukov
2 years ago

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

In 53379:

Query: Restore late compact() call for the posts_clauses_request filter.

This addresses a backward compatibility break where posts_join_request and other filters were applied, but their results were subsequently discarded and earlier values were used instead.

Follow-up to [52974], [53175].

Props 5um17, johnbillion, peterwilsoncc, hellofromTonya, SergeyBiryukov.
Merges [53370] and [53375] to the 6.0 branch.
Fixes #55699.

Note: See TracTickets for help on using tickets.