Make WordPress Core

Opened 22 months ago

Closed 20 months ago

Last modified 20 months ago

#56244 closed task (blessed) (fixed)

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

Reported by: sergeybiryukov's profile SergeyBiryukov Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 6.1 Priority: normal
Severity: normal Version:
Component: Comments Keywords: add-to-field-guide has-patch has-unit-tests
Focuses: docs, coding-standards Cc:

Description

Background: #56243

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

  • Some functions use $comment_id or $comment_ID, which should be renamed to $comment.
  • Some functions can accept a WP_Comment object, but it's not explicitly documented:
    • get_page_of_comment()
    • wp_new_comment_notify_moderator()
    • wp_new_comment_notify_postauthor()
    • wp_notify_moderator()

This ticket aims to bring some consistency.

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)

Attachments (3)

56244.diff (4.4 KB) - added by SergeyBiryukov 22 months ago.
56244.2.diff (4.5 KB) - added by SergeyBiryukov 22 months ago.
56244.3.diff (4.7 KB) - added by SergeyBiryukov 22 months ago.

Download all attachments as: .zip

Change History (28)

#1 @SergeyBiryukov
22 months ago

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

#2 follow-up: @audrasjb
22 months ago

  • Keywords 2nd-opinion added

@SergeyBiryukov question: shouldn't we keep $comment_id so it's more clear than just $comment?

In my –very humble – opinion, $comment would be reserved for WP_Comment object, just like $post and $post_id. What do you think?

I'm not opposed to your proposal, though. Just my two cents :)

#3 in reply to: ↑ 2 ; follow-up: @SergeyBiryukov
22 months ago

Replying to audrasjb:

shouldn't we keep $comment_id so it's more clear than just $comment?
In my –very humble – opinion, $comment would be reserved for WP_Comment object, just like $post and $post_id. What do you think?

Thanks for the comment! To me, it's confusing when $comment_id can mean two different things:

  • A comment ID.
  • A comment object.

I would like to separate these, so that:

  • $comment_id always means a (numeric) comment ID.
  • $comment as a function parameter means either a comment ID or comment object, but is typically converted to an object within the first lines of the function.

This is already the case for the majority of the functions, and is consistent with the majority of post functions with a $post parameter that accept a post ID or post object (as noted in #56243).

This ticket aims to bring some consistency to the remaining functions :)

This is also relevant in the context of PHP 8 named parameters, which WordPress does not support right now, but should eventually, and by that time renaming any parameters would be a backward compatibility break, so I'm trying to clean this up while we still can :)

#4 in reply to: ↑ 3 @audrasjb
22 months ago

  • Keywords 2nd-opinion removed

Replying to SergeyBiryukov:

Thanks for the comment! To me, it's confusing when $comment_id can mean two different things:

  • A comment ID.
  • A comment object.

I would like to separate these, so that:

  • $comment_id always means a (numeric) comment ID.
  • $comment as a function parameter means either a comment ID or comment object, but is typically converted to an object within the first lines of the function.

It totally makes sense to me now! thanks for the clarification 🙏

#5 follow-up: @peterwilsoncc
22 months ago

Something I hit working on a plugin is that sometimes $comment_ID is subsequently used in a compact() for functions taking an array of arguments.

This makes the case of the ID important for backward compatibility.

#6 @SergeyBiryukov
22 months ago

In 53719:

Coding Standards: Improve variable names in wp-trackback.php.

This fixes a Variable "$comment_post_ID" is not in valid snake_case format WPCS warning.

$comment_post_ID, while matching the database field of the same name, does not follow the WordPress coding standards, so the variable is now renamed to $comment_post_id.

Additionally, these variables are renamed for clarity:

  • $tb_id to $post_id (this was not the trackback ID)
  • $tb_url to $trackback_url

Follow-up to [172], [637], [1616],

See #55647, #56244.

#7 @SergeyBiryukov
22 months ago

In 53723:

Coding Standards: Rename $comment_post_ID and $comment_author_IP variables in various files.

This fixes two WPCS warnings:

  • Variable "$comment_post_ID" is not in valid snake_case format
  • Variable "$comment_author_IP" is not in valid snake_case format

While matching the database fields of the same name, these variables did not follow the WordPress coding standards, and are now renamed to address that.

Note: The name change only affects internal variables and parameters for a few actions receiving a comment post ID:

  • edit_comment
  • comment_id_not_found
  • comment_closed
  • comment_on_trash
  • comment_on_draft
  • comment_on_password_protected
  • pre_comment_on_post

The change does not affect parameters for functions receiving an array of comment data:

  • wp_insert_comment()
  • wp_new_comment()
  • wp_update_comment()
  • wp_handle_comment_submission()

The associated array keys still match the database fields: comment_post_ID and comment_author_IP.

Follow-up to [1706], [2894], [8720], [28427], [28437], [28457], [34799], [53720],

See #55647, #56244.

#8 @SergeyBiryukov
22 months ago

In 53729:

Coding Standards: Standardize on user_id when passing data to comment functions.

The wp_new_comment(), wp_update_comment(), and wp_filter_comment() functions already normalize the user_ID parameter internally to user_id, which matches the database field name.

This commit aims to bring some consistency when passing the parameter in core.

The corresponding $user_ID variable is also renamed to $user_id to match the other variables when not referring to the $user_ID global, which has an exception in the WordPress coding standards.

Follow-up to [8543], [8720], [12267], [12300], [26491], [28915], [28922], [34799], [49303].

See #55647, #56244.

#9 in reply to: ↑ 5 ; follow-up: @SergeyBiryukov
22 months ago

Replying to peterwilsoncc:

Something I hit working on a plugin is that sometimes $comment_ID is subsequently used in a compact() for functions taking an array of arguments.

This makes the case of the ID important for backward compatibility.

Thanks! Yes, the intention here is to fully maintain backward compatibility.

I've been thinking about opening a separate ticket for normalizing other parameters in the same way as it was done for user_IDuser_id in [12267], [12300], and [28915]:

  • comment_IDcomment_id
  • comment_post_IDcomment_post_id
  • comment_author_IPcomment_author_ip

Then any combination of these parameters would work regardless of the case.

Last edited 22 months ago by SergeyBiryukov (previous) (diff)

#10 @SergeyBiryukov
22 months ago

In 53730:

Coding Standards: Remove extra comma in a compact() call.

This fixes a PHP compatibility error in wp_handle_comment_submission():

  • Trailing comma's are not allowed in function calls in PHP 7.2 or earlier

Follow-up to [53729].

See #55647, #56244.

#11 @SergeyBiryukov
22 months ago

In 53732:

Tests: Correct the test for passing all expected parameters to the preprocess_comment filter.

Previously, assertSame() was replaced with assertSameSets(), which does not preserve the array keys. assertSameSetsWithIndex() should have been used instead.

Follow-up to [36038], [48937], [53729], [53730].

See #55647, #56244.

#12 in reply to: ↑ 9 @SergeyBiryukov
22 months ago

Replying to SergeyBiryukov:

I've been thinking about opening a separate ticket for normalizing other parameters in the same way as it was done for user_IDuser_id in [12267], [12300], and [28915]:

  • comment_IDcomment_id
  • comment_post_IDcomment_post_id
  • comment_author_IPcomment_author_ip

Then any combination of these parameters would work regardless of the case.

Created: #56261

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


22 months ago

#14 follow-up: @peterwilsoncc
22 months ago

[53729] affects the data sent to the preprocess_comment filter.

Prior to this change the user ID would be sent to the filter both as user_ID and user_id when called from the modified function. After the change this is no longer the case.

Based on a quick search of the plugin repo I think this will affect some popular plugins.

POC test for wp_ajax_replyto_comment(), add this method to the test class Tests_Ajax_ReplytoComment:

<?php
public function test_filter_args() {
        // Become an administrator.
        $this->_setRole( 'administrator' );

        // Get a comment.
        $comments = get_comments(
                array(
                        'post_id' => self::$comment_post->ID,
                )
        );
        $comment  = array_pop( $comments );

        $filter = new MockAction();
        add_filter( 'preprocess_comment', array( $filter, 'filter' ) );

        // Set up a default request.
        $_POST['_ajax_nonce-replyto-comment'] = wp_create_nonce( 'replyto-comment' );
        $_POST['comment_ID']                  = $comment->comment_ID;
        $_POST['content']                     = 'Lorem ipsum dolor sit amet, consectetur adipiscing elit.';
        $_POST['comment_post_ID']             = self::$comment_post->ID;

        // Make the request.
        try {
                $this->_handleAjax( 'replyto-comment' );
        } catch ( WPAjaxDieContinueException $e ) {
                unset( $e );
        }

        // Check the filter was called with user_ID.
        $args = $filter->get_args()[0][0];
        $this->assertArrayHasKey( 'user_ID', $args );
}

#15 in reply to: ↑ 14 @SergeyBiryukov
22 months ago

Replying to peterwilsoncc:

[53729] affects the data sent to the preprocess_comment filter.

Prior to this change the user ID would be sent to the filter both as user_ID and user_id when called from the modified function. After the change this is no longer the case.

Good catch, thanks!

There was a similar test added for wp_handle_comment_submission() in [36038] / #34997, but it looks like wp_ajax_replyto_comment() does not use that function and just calls wp_new_comment() directly. It could perhaps use wp_handle_comment_submission() instead of partially duplicating it, but that would be a separate enhancement.

56244.diff fixes the issue in my testing and includes a unit test for wp_new_comment().

#16 follow-up: @peterwilsoncc
22 months ago

I'll try to do some testing of re-normalization after the filter.

With the patch applied isset( $commentdata['user_ID'] ) will always return true after the filter, which wasn't necessarily the case before. My reading of the code is that once the filter runs, user_ID will take priority over user_id which seems the opposite of what you want here.

Determining whether a plugin has modified the user ID seems complicated, sadly.

#17 in reply to: ↑ 16 ; follow-up: @SergeyBiryukov
22 months ago

Replying to peterwilsoncc:

With the patch applied isset( $commentdata['user_ID'] ) will always return true after the filter, which wasn't necessarily the case before. My reading of the code is that once the filter runs, user_ID will take priority over user_id which seems the opposite of what you want here.

Thanks! It looks like setting both fields in the re-normalization may have been redundant:

$commentdata['user_ID'] = (int) $commentdata['user_ID'];
$commentdata['user_id'] = $commentdata['user_ID']

56244.2.diff replaces that with:

$commentdata['user_id'] = (int) $commentdata['user_ID'];

#18 in reply to: ↑ 17 ; follow-up: @peterwilsoncc
22 months ago

Replying to SergeyBiryukov:

Thanks! It looks like setting both fields in the re-normalization may have been redundant:

I am beginning to think we need WP_Case_Insensitive_Array_Like_Object (but that too would break plugins using is_array()). :)

It appears some plugins are expecting user_id on the filter, other user_ID and there is no nice way for core to determine accurately which ought to be preferred after the filter is run.

Re: my comment yesterday

With the patch applied isset( $commentdata['user_ID'] ) will always return true after the filter...

It occurred to me overnight that I was wrong. If a plugin reconstructs the array in it's entirety using user_id then this will be false.

#19 in reply to: ↑ 18 @SergeyBiryukov
22 months ago

Replying to peterwilsoncc:

Re: my comment yesterday

With the patch applied isset( $commentdata['user_ID'] ) will always return true after the filter...

It occurred to me overnight that I was wrong. If a plugin reconstructs the array in it's entirety using user_id then this will be false.

Thinking about this some more, it might be beneficial for the comment_post action to also consistently receive both user_id and user_ID, as does the preprocess_comment filter.

56244.3.diff implements that, bringing consistency between the two normalization fragments. A similar approach could then be used for other fields with mixed case names in #56261.

#20 @peterwilsoncc
22 months ago

Thanks @SergeyBiryukov, this looks good to me based on a read through of the code.

In the new test you can remove the call to remove_filter() as that is done during the tearDown automatically.

#21 @milana_cap
20 months ago

  • Keywords add-to-field-guide added

#22 @SergeyBiryukov
20 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.

This ticket was mentioned in PR #3438 on WordPress/wordpress-develop by SergeyBiryukov.


20 months ago
#23

  • Keywords has-patch has-unit-tests added

#24 @SergeyBiryukov
20 months ago

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

In 54489:

Comments: Consistently normalize user_ID to user_id in wp_new_comment().

For backward compatibility, the user_id parameter of wp_new_comment() can be spelled as user_ID, and plugins utilizing the preprocess_comment filter or the comment_post action should be able to receive both variations.

Follow-up to [12267], [12300], [28915], [36038], [53729].

Props peterwilsoncc, SergeyBiryukov.
Fixes #56244.

SergeyBiryukov commented on PR #3438:


20 months ago
#25

Merged in r54489.

Note: See TracTickets for help on using tickets.