Make WordPress Core

Opened 21 months ago

Closed 18 months ago

Last modified 18 months ago

#56243 closed task (blessed) (fixed)

Use a consistent parameter name for functions accepting a post ID or post object

Reported by: sergeybiryukov's profile SergeyBiryukov Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 6.1 Priority: normal
Severity: normal Version:
Component: Posts, Post Types Keywords: has-patch
Focuses: docs, coding-standards Cc:

Description

Most functions that accept a post ID or post object use $post as the parameter name.

  • Some functions use $post_id or just $id, which should be renamed to $post.
  • Some functions can accept a WP_Post object, but it's not explicitly documented:
    • get_the_category()
    • get_the_category_list()
    • the_category()
    • get_the_tag_list()
    • get_the_term_list()
    • the_terms()

This ticket aims to bring some consistency.

Change History (13)

#1 @SergeyBiryukov
21 months ago

  • Owner set to SergeyBiryukov
  • Status changed from new to accepted

#2 @SergeyBiryukov
21 months ago

In 53715:

Posts, Post Types: Standardize on $post parameter name where appropriate.

This renames the $post_id or $id parameters to $post for functions that accept a post ID or post object:

  • get_sample_permalink()
  • get_sample_permalink_html()
  • wp_check_post_lock()
  • wp_set_post_lock()
  • get_the_tags()
  • comment_class()
  • get_comment_class()
  • get_comments_link()
  • get_comments_number()
  • comments_number()
  • get_comments_number_text()
  • comments_open()
  • pings_open()
  • comment_form()
  • do_trackbacks()
  • pingback()
  • post_permalink()
  • get_post_permalink()
  • get_edit_post_link()
  • edit_post_link()
  • get_delete_post_link()
  • post_class()
  • get_post_class()
  • the_attachment_link()
  • wp_get_attachment_link()
  • wp_list_post_revisions()
  • check_and_publish_future_post()
  • add_ping()
  • get_pung()
  • get_to_ping()
  • wp_get_post_revisions()
  • wp_get_post_revisions_url()

Additionally, $revision_id is renamed to $revision in:

  • wp_restore_post_revision()
  • wp_delete_post_revision()

Includes minor documentation improvements for consistency and code layout fixes for better readability.

Follow-up to [1599], [1794], [2881], [3303], [3851], [5302], [6633], [6716], [6985], [7103], [7149], [7747], [8011], [8638], [8643], [8695], [9138], [9273], [11425], [11922], [11956], [12284], [12810], [12923], [13023], [13171], [25567], [27156], [27473], [28558], [28602], [33659], [38852], [47276], [47366], [48622], [49544], [49597], [52095].

See #56243, #55647.

#3 follow-up: @pbiron
21 months ago

There are also a lot of occurrences of $post_ID (some of which need to be an ID and not a post object, so not sure if they all should be changed to $post).

I can provide a list of those occurrences if needed...but I suspect @SergeyBiryukov is better at searching the code base than I am :-)

Not sure if it would be "scope creep" for this ticket, there are also mix of $comment_ID and $comment_id throughout code. Should those be standardized as well?

#4 in reply to: ↑ 3 ; follow-up: @SergeyBiryukov
21 months ago

Replying to pbiron:

There are also a lot of occurrences of $post_ID (some of which need to be an ID and not a post object, so not sure if they all should be changed to $post).

I think those that do not accept a WP_Post object should be renamed to $post_id for consistency.

I can provide a list of those occurrences if needed...

That would be helpful, thanks!

Not sure if it would be "scope creep" for this ticket, there are also mix of $comment_ID and $comment_id throughout code. Should those be standardized as well?

Indeed, I've started looking into that too and will open a similar ticket for those :) It's worth noting that $comment_ID causes a WPCS warning and should be fixed anyway:

| WARNING | [ ] Variable "$comment_ID" is not in valid snake_case
|         |     format, try "$comment_i_d"
|         |     (WordPress.NamingConventions.ValidVariableName.VariableNotSnakeCase)

#5 in reply to: ↑ 4 ; follow-up: @pbiron
21 months ago

Replying to SergeyBiryukov:

Replying to pbiron:

I can provide a list of those occurrences if needed...

That would be helpful, thanks!

Will do that in the next few days.

Indeed, I've started looking into that too and will open a similar ticket for those :) It's worth noting that $comment_ID causes a WPCS warning and should be fixed anyway:

| WARNING | [ ] Variable "$comment_ID" is not in valid snake_case
|         |     format, try "$comment_i_d"
|         |     (WordPress.NamingConventions.ValidVariableName.VariableNotSnakeCase)

The WPCS errors are what brought those to my attention. In plugins I write, when I hook into actions/filters, I try to name the parameters to my callbacks exactly the same as in the core hook DocBlock...and I often have to add // phpcs:ignore WordPress.NamingConventions.ValidVariableName.VariableNotSnakeCase to silences those warnings (and I've been working on one such plugin that deals with comments lately :-).

I'll keep an eye out for that other ticket being opened. thanx!

#6 in reply to: ↑ 5 @SergeyBiryukov
21 months ago

Replying to pbiron:

I'll keep an eye out for that other ticket being opened. thanx!

Created: #56244

#7 @peterwilsoncc
21 months ago

The change to comment_form() appears to change the behaviour.

Previously if an invalid post ID was passed to the function comments_open() would return false and no comment form be displayed.

The new conditional $post_id = $post ? $post->ID : get_the_ID(); means that if an invalid post ID is passed to the comment_form(), the form will be display for the current global post.

This ticket was mentioned in PR #2988 on WordPress/wordpress-develop by peterwilsoncc.


21 months ago
#8

  • Keywords has-patch added

See https://core.trac.wordpress.org/ticket/56243#comment:7

This restores how comment_form() behaves if an invalid post ID is passed to the function.

I tested by making HTTP requests with the following mini plugin.

{{{php
<?php

/

  • Comment form test. */

return;

add_action(

'wp_head',
function() {

$testing = 1; also try null, 50000000
comment_form( [], $testing );
exit;

},
5

);

add_action(

'comment_form_comments_closed',
function() {

var_dump( 'doing comment_form_comments_closed' );

},
5

);
}}}

#9 @peterwilsoncc
21 months ago

The linked pull request:

  • restores the behaviour of comment_form()
  • updates the description of the comment_form_comments_closed action to note it fires with an invalid post ID
  • includes some tests to determine the form displays as expected.

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


21 months ago

#11 @SergeyBiryukov
19 months ago

  • Type changed from enhancement to task (blessed)

Converting this to a task, as these are mostly coding standards fixes, and there are some follow-up changes to make.

#12 @SergeyBiryukov
18 months ago

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

In 54488:

Comments: Return early from comment_form() if an invalid post ID is passed.

If an invalid post ID is passed to the function, comments_open() should return false, and no comment form be displayed. This commit restores the previous behavior that was unintentionally changed when standardizing on the $post parameter name.

Follow-up to [53715].

Props peterwilsoncc.
Fixes #56243.

SergeyBiryukov commented on PR #2988:


18 months ago
#13

Thanks for the PR! Merged in r54488.

Note: See TracTickets for help on using tickets.