Make WordPress Core

Opened 2 months ago

Closed 5 weeks ago

Last modified 5 weeks ago

#61307 closed defect (bug) (fixed)

Wrong variable name in filter documentation

Reported by: davidbinda's profile david.binda Owned by: hellofromtonya's profile hellofromTonya
Milestone: 6.6 Priority: normal
Severity: normal Version: 6.0
Component: Query Keywords: has-patch commit
Focuses: docs Cc:

Description

In r52974 some $pieces variables were changed to $clauses including the usage in docblocks. However, some of the instances were reverted back to $pieces in r53376 but the docblocks were intact creating possibly mis-leading docblocks.

I'd like to propse to change the docblock back to the original state prior r52974

Attachments (1)

61307.diff (2.7 KB) - added by david.binda 2 months ago.

Download all attachments as: .zip

Change History (15)

@david.binda
2 months ago

#1 @audrasjb
2 months ago

  • Keywords has-patch added
  • Milestone changed from Awaiting Review to 6.6
  • Owner set to audrasjb
  • Status changed from new to reviewing

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


8 weeks ago

#3 @oglekler
8 weeks ago

  • Focuses docs added

#4 @SergeyBiryukov
8 weeks ago

Thanks for the ticket!

IIRC, this part of the DocBlock was not reverted back to $pieces in [53376] specifically to avoid confusion. My thinking was that $clauses represents compact( $pieces ), which gets passed to the filters, not $pieces itself.

So reverting this to $pieces would be more confusing for me, but I'm curious to see what others think.

Another thing to consider is that these variable names are often copied when writing new functions attached to the hooks, and $clauses seems like a more fitting variable name.

Last edited 8 weeks ago by SergeyBiryukov (previous) (diff)

#5 @audrasjb
8 weeks ago

  • Type changed from enhancement to defect (bug)

#6 @ironprogrammer
7 weeks ago

I agree that $clauses more accurately describes the associative array expected here after compact( $pieces ), but it does look funny for the comment to not allude to the obvious use of $pieces in context.

The DocBlock at https://core.trac.wordpress.org/browser/trunk/src/wp-includes/class-wp-query.php#L2934 is verbose in describing what $clauses represents. Would it help for the other filter calls to include these details to avoid ambiguity?

Last edited 7 weeks ago by ironprogrammer (previous) (diff)

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


5 weeks ago

#8 @hellofromTonya
5 weeks ago

  • Keywords changes-requested added

Interesting. Looking at the history:

  • [52974] changed $pieces to $clauses.
  • [53375] for WP_Query, reverted part of the change for WP_Query -> the individual query pieces array gets assigned to $pieces and compact( $pieces ) represents the $clauses.
  • [53376] did the same treatment in WP_Comment_Query, WP_Network_Query, WP_Site_Query, and WP_Term_Query.

Each of these queries are now consistent, which is good.

IIRC, this part of the DocBlock was not reverted back to $pieces in [53376] specifically to avoid confusion. My thinking was that $clauses represents compact( $pieces ), which gets passed to the filters, not $pieces itself.

So reverting this to $pieces would be more confusing for me, but I'm curious to see what others think.

I agree with @SergeyBiryukov and @ironprogrammer. Pieces and clauses are 2 different things. The naming is consistent and aligns well IMO to these 2 different purposes.

The DocBlock at https://core.trac.wordpress.org/browser/trunk/src/wp-includes/class-wp-query.php#L2934 is verbose in describing what $clauses represents. Would it help for the other filter calls to include these details to avoid ambiguity?

I agree with @ironprogrammer. The filter's DocBlock for the others should also receive the more verbose version for the $clauses. Doing so keeps them consistent, helps with understanding, and should avoid confusion.

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


5 weeks ago
#9

For WP_Comment_Query, WP_Network_Query, and WP_Site_Query, expands the $clauses array structure explanation in filter DocBlock.

Benefits:

  • Consistency with WP_Query and WP_Term_Query.
  • Helps to improve understanding.
  • Should help to avoid confusion of the purpose for pieces and clauses.

Follow-up to r52974, r53375, and r53376.

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

#10 @hellofromTonya
5 weeks ago

  • Keywords changes-requested removed

Patch: https://github.com/WordPress/wordpress-develop/pull/6869

Now available for review. What do you think?

#11 @hellofromTonya
5 weeks ago

  • Keywords commit added
  • Owner changed from audrasjb to hellofromTonya

Patch: https://github.com/WordPress/wordpress-develop/pull/6869

With an approval on the PR (thank you @johnbillion), marking for commit. Reassigning to me, as I'm preparing the commit.

#12 @hellofromTonya
5 weeks ago

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

In 58454:

Docs: Document $clauses associative array in *-clauses filters.

Document each element within the filter parameter $clauses associative array structure for the 'comments_clauses', 'networks_clauses', and 'sites_clauses' filters.

This change:

  • Brings consistency amongst the WP_[*_]Query filters.
  • Helps to improve understanding.
  • Helps to avoid confusion of the purpose for pieces and clauses.

Follow-up to [53376], [53375], [52974].

Props david.binda, audrasjb, hellofromTonya, ironprogrammer, johnbillion, oglekler, SergeyBiryukov.
Fixes #61307.

#14 @hellofromTonya
5 weeks ago

Hello @davidbinda,

Do you agree with discussion and [58454]?

If no, please feel free to re-open this ticket to continue the discussion.

If yes, woohoo.

Thanks :)

Note: See TracTickets for help on using tickets.