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

[SergeyBiryukov]

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)

[audrasjb]

@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 :)

[SergeyBiryukov]

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 :)

[audrasjb]

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 🙏

[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.

[SergeyBiryukov]

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.

[SergeyBiryukov]

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.

[SergeyBiryukov]

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.

[SergeyBiryukov]

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_ID → user_id in [12267], [12300], and [28915]:

• comment_ID → comment_id
• comment_post_ID → comment_post_id
• comment_author_IP → comment_author_ip

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

[SergeyBiryukov]

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.

[SergeyBiryukov]

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.

[SergeyBiryukov]

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_ID → user_id in [12267], [12300], and [28915]:

• comment_ID → comment_id
• comment_post_ID → comment_post_id
• comment_author_IP → comment_author_ip

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

Created: #56261

[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.

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 );
}

[SergeyBiryukov]

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().

[peterwilsoncc]

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.

[SergeyBiryukov]

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'];

[peterwilsoncc]

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.

[SergeyBiryukov]

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.

[peterwilsoncc]

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.

[SergeyBiryukov]

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

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

[SergeyBiryukov]

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.

Merged in r54489.