Make WordPress Core

Opened 3 years ago

#53842 new defect (bug)

Review the type of select return values

Reported by: jrf's profile jrf Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: Build/Test Tools Keywords: 2nd-opinion
Focuses: rest-api Cc:

Description

In addressing ticket #46149, four tests were encountered which were testing (part of) a return value of WP Core functions, but were found to be using the wrong value type in this comparison.

This was discovered due to the fact that the assertContains() method in PHPUnit uses strict type comparisons as of PHPUnit 8.0.2.

For the time being, the tests will be updated to reflect the REAL return type per proposed commit https://github.com/jrfnl/wordpress-develop-official/commit/64dd09d5c292c3eac80419d5a1a325dab453a5ed

This ticket is being opened to follow up on this as it should be investigated whether the return type of these WP Core function as-is is actually correct and the test update was therefore justified. Or whether the test update should be reverted and the actual WP Core functions should be updated.

Details

Tests_Comment_GetPageOfComment::test_page_number_when_unapproved_comments_are_included_for_current_commenter() and Tests_Comment_GetPageOfComment::test_page_number_when_unapproved_comments_are_included_for_current_user()

Both these tests generate a comment ID using a test Factory class:

<?php
$new_unapproved = self::factory()->comment->create(
        $comment_args
);

$new_unapproved will now contain a comment ID as an integer.
This integer is expected to be included in a list retrieved via the `get_comments()` function and filtered via `wp_list_pluck()`.

Based on these two tests, the comment_ID as returned in the array retrieved via get_comments() is a string, not an integer.

So the questions for these two tests are:

  • Should the comment_ID returned by get_comments() be a string or an integer ?
  • If the get_comments() function should remain unchanged, should the TestCase::factory()->comment->create()` method be adjusted to return a string for the ID value instead of an integer ?
  • If the answer to both the above questions is "no", no further action is needed once the proposed commit mentioned above has been committed.

WP_Test_REST_Post_Meta_Fields::test_set_value_multiple_custom_schema() and WP_Test_REST_Term_Meta_Fields::test_set_value_multiple_custom_schema()

These two tests both update the post meta of a WP post via a REST API POST request.
The data to be set is passed as integers:

<?php
$data = array(
        'meta' => array(
                'test_custom_schema_multi' => array( 2, 8 ),
        ),
);
$request->set_body_params( $data );
$response = rest_get_server()->dispatch( $request );

The test subsequently requests the post meta information via a call to `get_post_meta()` and ensures that the returned array contains both values.
The values returned in the array from get_post_meta(), however are strings, not integers.

So the questions for these two tests are:

  • Where does the type change from integer to string happen ?
  • Is the data when set via the REST API being saved correctly ?
  • Does the Core code need to change or is the test update correct ?

In this case, I suspect the type change may be due to the fact that all data received from $_POST will always be in string format, however, the handling of these API requests should be investigated to be sure.

Change History (0)

Note: See TracTickets for help on using tickets.