-
Notifications
You must be signed in to change notification settings - Fork 156
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Change how the New
tab is generated on the Pull screen for external connections
#811
Conversation
…l connections. When making a request for new content with previously pulled or skipped items and when using an external connection, use this new endpoint via a POST request
@dkotter its probably worthwhile to add some explicit comments in here as to why we took this approach especially for the case where VIP or someone else might flag it as a concern (thus there would be some comments for them to reference and better understand the why maybe even with a link to the PR or issue with our analysis and stats on the topic). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dkotter Awesome work! 🎉 This is looking good to me. I just have some minor comments, please take a look : )
includes/rest-api.php
Outdated
} | ||
|
||
// Filter documented in includes/classes/ExternalConnections/WordPressExternalConnection.php | ||
$query = new \WP_Query( apply_filters( 'dt_remote_get_query_args', $args, $request ) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain why do we need this filter in the callback, which will be executed on the remote sites, not from the site making the request?
Also, how can we distinguish between normal remote_get
calls and pull content calls? I'm thinking about checking for the existence of the third argument.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've gone ahead and removed this filter as it isn't needed. We run the same filter before we end up making our remote POST request, so this would just result in filtering the same values twice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And after thinking about it a bit more, not sure it makes sense to run the first filter, as that's meant to filter the remote_get query args. I've moved that code to run after our remote post call and added a new filter in the remote post method around those arguments. So to answer your second question, if you only want to filter the arguments for a Pull call, you'd use this new filter instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🎉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure why I didn't approve this.
Description of the Change
When building the content that shows on the
New
tab of the Pull screen, we determine what content has already been pulled and what content has already been skipped. This is then trimmed down to 200 items and used in apost__not_in
query.This can cause two different problems:
New
tabThis PR attempts to fix both of those problems in the following ways:
New
tab on the Pull screenPOST
request to this endpoint (instead ofGET
), which allows us to pass data via thebody
parameter instead of in the URLbody
Because we remove that 200 item limit, we won't run into issues with previously pulled or skipped items showing on the
New
tab. Fixes #808Because we are using a custom endpoint with a
POST
request, fixes potential timeout issues when on a WordPress.com VIP site because we no longer have the strict 3 second timeout limit. Fixes #809Alternate Designs
Ultimately we are still running a
post__not_in
query, which may not be the most performant. Other approaches were considered but ultimately rejected:post__not_in
, increase ourposts_per_page
value and then remove excluded items after. This can still cause performance issues and causes pagination issuesposts_per_page
, could set an upper limit and break out into new queries once that limit is reached. Still have pagination problems and could still have performance issues because we are running more queriesBenefits
All excluded items are properly excluded from the
New
tab. Better performance in certain situationsPossible Drawbacks
I can't think of any but maybe some drawback around switching from a
GET
request to aPOST
request?Verification Process
Checklist:
Applicable Issues
#808
#809
Changelog Entry