Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 7 months ago

#50617 closed enhancement (fixed)

REST API: Add modified_before and modified_after to WP_REST_Request attributes

Reported by: claytoncollie's profile claytoncollie Owned by: johnbillion's profile johnbillion
Milestone: 5.7 Priority: normal
Severity: normal Version: 4.7
Component: REST API Keywords: has-patch has-dev-note
Focuses: Cc:

Description

Right now there are two attributes in WP_REST_Request that allow requests to be made by the Post's published date; before and after.

I am proposing that modified_before and modified_after be added to allow for querying posts by their modified date.

Here is the list of the current attributes.

https://developer.wordpress.org/rest-api/requests/#attributes

before and after are on lines 64 and 44 of this gist.

Change History (24)

#1 @SergeyBiryukov
4 years ago

  • Keywords needs-patch needs-unit-tests added

#2 @SergeyBiryukov
4 years ago

Hi there, welcome to WordPress Trac! Thanks for the ticket.

This seems like a good idea, would you be interested in working on a patch?

#3 @claytoncollie
4 years ago

@SergeyBiryukov Thanks for the response. I am happy to work on a patch.

This ticket was mentioned in PR #501 on WordPress/wordpress-develop by claytoncollie.


4 years ago
#4

  • Keywords has-patch has-unit-tests added; needs-patch needs-unit-tests removed

Code and tests for adding an additional query parameter to the REST API request.

Added two new arguments, modified_before and modified_after which request data based on the post_modified field from the WP_Post object.

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

#5 @claytoncollie
4 years ago

  • Keywords needs-patch needs-unit-tests added; has-patch has-unit-tests removed

@SergeyBiryukov I wrote a patch for this ticket. Happy to make changes as you see fit. I do have a couple of questions.

  1. I am using the post_modified field for the request. Should I use post_modified_gmt instead?

https://core.trac.wordpress.org/browser/tags/5.5/src/wp-includes/class-wp-post.php#L151

WP_Query date_query defaults to post_date so I think post_modified is correct.

https://developer.wordpress.org/reference/classes/wp_query/#date-parameters

  1. I ported my changes to the Comments controller as well since it uses the same request arguments. Is that alright for the scope of this ticket?
  1. I wrote tests for Posts, Pages, and Attachments since they all used similar request/test methods. Is that alright for the scope of this ticket?
  1. Do I need to open a PR to update the documentation at https://github.com/WP-API/docs/blob/master/requests.md

#6 @claytoncollie
4 years ago

@SergeyBiryukov Do I need to do anything to move this along? Thanks.

#7 @claytoncollie
4 years ago

@TimothyBlynJacobs Hi, I was waiting for open floor call on the slack meeting on 9/10/2020, but might have missed it. I was on another work meeting at the same time. Can you and I go over this ticket at some point?

#8 @claytoncollie
4 years ago

  • Keywords has-patch dev-feedback added; needs-patch needs-unit-tests removed

TimothyBJacobs commented on PR #501:


4 years ago
#9

Sorry for the back and forth on this @claytoncollie. WP_Date_Query does support an array of clauses, so why can't we support searching by both columns at once actually?

claytoncollie commented on PR #501:


4 years ago
#10

@TimothyBJacobs Not a problem at all. I want to get the right so I appreciate you looking into it. I looked at WP_Date_Query and the related tests where a person is querying by post_date and post_modified date at the same time. The first date array is built with the post_date data then a second array is created to house the post_modified data. The reason I put the conditional in my PR is that we are setting the first array $args['date_query'][0] in both instances.

If you like, I can write some more code to support where they work individually and where they work together. This is the first pass but it feels weird. It also doesn't support when somebody is querying by after and modified_before at the same time. Only after and modified_after and then before and modified_before`. The combinations start to get out of hand.

{{{php
Set modified_before into date query. Date query must be specified as an array of an array.
if ( isset( $registeredmodified_before?, $requestmodified_before? ) ) {

Check if we are using modified_before and before in the same query.
if ( isset( $registeredbefore?, $requestbefore? ) ) {

$argsdate_query?[1]before? = $requestmodified_before?;
$argsdate_query?[1]column? = 'post_modified';

} else {

$argsdate_query?[0]before? = $requestmodified_before?;
$argsdate_query?[0]column? = 'post_modified';

}

}
}}}

and

{{{php
Set modified_after into date query. Date query must be specified as an array of an array.
if ( isset( $registeredmodified_after?, $requestmodified_after? ) ) {

Check if we are using modified_after and after in the same query.
if ( isset( $registeredafter?, $requestafter? ) ) {

$argsdate_query?[1]after? = $requestmodified_after?;
$argsdate_query?[1]column? = 'post_modified';

} else {

$argsdate_query?[0]after? = $requestmodified_after?;
$argsdate_query?[0]column? = 'post_modified';

}

}
}}}

TimothyBJacobs commented on PR #501:


4 years ago
#11

Sorry for the delay in responding to your comment @claytoncollie.

Instead of trying to implement it in one clause, can we make each date check its own clause? Something like this?

{{{php
$date_query = array();

if ( isset( $registeredbefore?, $requestbefore? ) ) {

$date_query[] = array(

'before' => $requestbefore?,
'column' => 'post_date',

);

}

if ( isset( $registeredmodified_before?, $requestmodified_before? ) ) {

$date_query[] = array(

'before' => $requestmodified_before?,
'column' => 'post_modified',

);

}

if ( isset( $registeredafter?, $requestafter? ) ) {

$date_query[] = array(

'after' => $requestafter?,
'column' => 'post_date',

);

}

if ( isset( $registeredmodified_after?, $requestmodified_after? ) ) {

$date_query[] = array(

'after' => $requestmodified_after?,
'column' => 'post_modified',

);

}
}}}

claytoncollie commented on PR #501:


4 years ago
#12

Thanks for getting back to me @TimothyBJacobs

Your code is much more simple and builds the array properly. I updated the Post Controller with this code and it works perfectly.

I also removed the code I had for the Comments Controller as the WP_Comment class does not have a modified date as a class parameter.

The last thing I did with this PR is to add the modified_after and modified_before array keys to the test fixtures for testing the query params on Post, Page, and Attachments.

#13 @TimothyBlynJacobs
4 years ago

  • Milestone changed from Awaiting Review to 5.7
  • Version set to 4.7

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


4 years ago

#15 @hellofromTonya
4 years ago

Capturing notes from Timothy in today's core scrub:

It has a solid patch I think, could use additional eyes from people familiar with the Query component to see if there would be any unintended ramifications from splitting one Date Query clause to 2+

cc @johnbillion @SergeyBiryukov @boonebgorges for your eyes on this one.

#16 @johnbillion
4 years ago

For anybody who might be wondering, the post modified date is considered public information so it's safe to add these as new REST API query parameters. The modified date is exposed in the modified and modified_gmt fields in an unauthenticated REST API request, and in the <updated> element in an Atom feed.

#17 @johnbillion
4 years ago

  • Keywords dev-feedback removed

The post_modified and post_modified_gmt fields in the database are not indexed. I've run into performance problems in the past when querying by these fields, and ended up adding a new index similar to type_status_date that used post_modified in place of post_date. Worth mentioning in the dev note at least so users are aware.

With the change to the date query structure there's a slight change to the resulting SQL when querying for both before and after. It does not functionally alter the query. The > and < order is reversed and a surrounding bracket pair is removed.

Before:

SELECT SQL_CALC_FOUND_ROWS wp_posts.ID
FROM wp_posts
WHERE 1=1
AND (
    (
        wp_posts.post_date > '2020-01-01 00:00:00'
        AND wp_posts.post_date < '2020-12-31 23:59:59'
    )
)
AND wp_posts.post_type = 'post'
AND ((wp_posts.post_status = 'publish'))
ORDER BY wp_posts.post_date DESC
LIMIT 0, 10

After:

SELECT SQL_CALC_FOUND_ROWS wp_posts.ID
FROM wp_posts
WHERE 1=1
AND (
    wp_posts.post_date < '2020-12-31 23:59:59'
    AND wp_posts.post_date > '2020-01-01 00:00:00'
)
AND wp_posts.post_type = 'post'
AND ((wp_posts.post_status = 'publish'))
ORDER BY wp_posts.post_date DESC
LIMIT 0, 10

I don't think this is a problem. WordPress doesn't guarantee that its SQL queries never change.

Going to look at why the CI is failing on the PR.

#18 @johnbillion
4 years ago

There were 3 failures:

1) WP_Test_REST_Attachments_Controller::test_get_items_valid_modified_date
Failed asserting that actual size 0 matches expected size 1.

/var/www/tests/phpunit/tests/rest-api/rest-attachments-controller.php:618

2) WP_Test_REST_Pages_Controller::test_get_items_valid_modified_date
Failed asserting that actual size 0 matches expected size 1.

/var/www/tests/phpunit/tests/rest-api/rest-pages-controller.php:404

3) WP_Test_REST_Posts_Controller::test_get_items_valid_modified_date
Failed asserting that actual size 0 matches expected size 1.

/var/www/tests/phpunit/tests/rest-api/rest-posts-controller.php:1519

@claytoncollie Wanna look into these? These look like sporadic test failures that only fail because of indeterminate sort order, but as far as I can tell those tests use specific and distinct dates.

Last edited 4 years ago by johnbillion (previous) (diff)

#19 @claytoncollie
4 years ago

Thanks for looking at this ticket @johnbillion

I found that if I created the post with the same post_date and post_modified date, the test would pass. I also found that if the post_date was some amount of time in the past compared to the post_modified date, then my tests would fail. For this reason, I split the post creation with the post_date value apart from the post_modified being updated. The post_date and post_modified dates are now 15 days apart. After doing it this way, all my tests pass.

Probably cannot have a post_modified value without a post_date value in the past. Does that sound correct?

#20 @johnbillion
4 years ago

  • Owner set to johnbillion
  • Status changed from new to reviewing

#21 @johnbillion
4 years ago

  • Keywords needs-dev-note added

#22 @johnbillion
4 years ago

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

In 50024:

REST API: Introduce modified_before and modified_after query parameters for the posts endpoints.

These parameters work just the same as before and after except they operate on the post modified date instead of the post published date.

Props claytoncollie, TimothyBlynJacobs, hellofromTonya

Fixes #50617

#24 @audrasjb
4 years ago

  • Keywords has-dev-note added; needs-dev-note removed
Note: See TracTickets for help on using tickets.