-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Add URL Details endpoint to REST API to allow retrieval of info about a remote URL #18042
Conversation
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.
@obenland Thanks for the changes! Really appreciate you taking this on and making it much better.
Left some small comments. Interested in what you think.
We need to wrangle some more feedback from the REST API group in Core.
We also need a rebase to make the playground pass (it's happened on all PRs recently).
Lastly, what do you think about this comment on the Issue?
2722e34
to
4c330d2
Compare
@kadamwhite We'd really appreciate your expertise and input on this if at all possible. |
@obenland Shall we try and move this forward in our next 20% time? What should we do to prepare for that so we can make progress at that time? |
Sure. I think double-checking #18042 (comment) would be the next step |
|
4c330d2
to
a5d9c25
Compare
Waiting on further feedback here. The outstanding issue blocking this from merging is the concern about exposing data about websites behind a firewall. Oh...and it looks like we have some PHPUnit test warnings @obenland. |
From what I could test, this doesn't expose data local data. Although I don't have a network behind a firewall etc, so I can't really comment about that. I tested this out and it worked fine. It did seem a bit odd the response wasn't actually JSON, just a string of the site's title. (I suppose returning JSON leaves the door open to returning other metadata about the site.) I wasn't aware of anywhere else in the REST API that just does that, but it's not the end of the world. |
3067b99
to
5967e70
Compare
Merged 🎉 I will raise followups to add extra functionality. Thanks to everyone for your help in getting this over the line (particularly @TimothyBJacobs and @swissspidy). |
Congrats @getdave! Nice work |
… a remote URL (#18042) * Scaffold out basic new endpoint * Implement basic retrival of title tag from remote url * Adds validation, sanitization and permissions checks. * i18n fixes and docblocks * Adds caching of remote request * Update with feedback * Tie up loose ends * Remove unneed replacements * Account for Custom Post Types in permissions check Addresses #18042 (comment) * Improve error handling * Improve args checking * Refactor to enable query of more data in future. * Use md5 Addresses #18042 (comment) * Extract utility functions * Fix lint errors * Add unit test scaffold * Attempt to mock HTTP requests for testing. Needs work. * Use DIR convention * Allow opt out of cache functionality via filter * Force tests to opt out of cache. Fix broken tests. * Adjust 404 case to use standard response code * Add dedicated methods to mock success/failure HTTP responses * Add test for empty body in response. This should trigger a failed response because we can’t read details from a URL which doesn’t provide a HTTP body. * Add ability to filter request args. Update tests to account. * Document cache filter * Iniital attempt at adding schema * Add test for unautenticated user. * Adjust schema to use WP convention * Adjust schema defaults * Add endpoint args schema test * Removes filter to bypass cache. This already exists in core as https://core.trac.wordpress.org/browser/tags/5.6/src/wp-includes/option.php#L785 * Use existing transient filter to test cache and bypass for majority of tests Fixes #18042 (comment) * Use default timeout. Addresses #18042 (comment) * Removes unnecessary default from schema Addresses #18042 (comment) * Use predefine constants for response size Resolves #18042 (comment) * Rearrange test code order * Adds ability to filter cache expiration Addresses #18042 (review) * Allow filtering the data retrived for a given URL Addresses #18042 (comment) * Remove @access comments Addresses #18042 (comment) * Prefer custom error message over get_status_header_desc Addresses #18042 (comment) * Utilise add_additional_fields_schema Addresses #18042 (comment) * Utilise add_additional_fields_to_object Addresses #18042 (comment) * Enable filtering of response object Filter on both cached and uncached response. * Add test to assert on ability to filter uncached responses * Rename url placeholder literal * Ensure accurately testing for correctly passed cached variable in filter args * Update to test filtering of both cached and uncached responses * Fixing linting * Use array offset syntax prefered by Core Addresses #18042 (comment) * Remove from cache and update filter See #18042 (comment) * Update filter name to align with standards Addresses #18042 (comment) * Update response code to align with Core standards Addresses #18042 (comment) * Update cache ttl in docs to use variable Addresses #18042 (comment) * Use comment to reference canonical version of filter docs Addresses #18042 (comment) * Update to cache entire HTTP response body and remove additional filter Addresses #18042 (comment) * Tweak code comments * Remove need to encode/decode cache value Co-authored-by: Konstantin Obenland <[email protected]> Add Bottom Sheet Select Control componet Add Bottom Sheet Select Control componet Remove file
@TimothyBJacobs At some point if this endpoint is accepted would I need to follow up with this in WordPess Core? |
Whenever the JS feature that uses this feature is slated to be included in Core, we'll create a Core ticket for merging the URL details endpoint. I can take care of merging it into Core. |
That's great thanks. Any chance I could get involved there? Only Ive not committed to WP Core and this seems like a good opportunity. |
Yeah, you can absolutely take the lead on that! |
Here is a follow up #28791 |
Closes #18047.
Adds a new REST API endpoint to grab details from a external URL. So now if I want to get the contents of the
title
tag onhttps://wordpress.org
I can send a request to this API endpoint and it will return the value in the response for use in the Gutenberg UI.Why might this be useful? See #19387.
As part of #17846 we need to display the contents of theThis has now long since shipped. Nonetheless, it might be a nice enhancement to be able to show details about the remote URL that has been selected.<title>
tag from the url the user has typed into the input field.This PR addresses this by adding a new experimental custom Gutenberg REST API endpoint. This endpoint:
parses the response in awe should investigate this in future (see here)DOMDocument
title
tag contents.To prevent abuse, the endpoint is authenticated and requires the user to have the
edit_posts
capability to be able to make a request.How has this been tested?
Automated Testing Instructions
npm run test-php
should run the unit tests which reside inphpunit/class-wp-rest-url-details-controller-test.php
.Manual Testing Instructions
http://localhost:8889/wp-admin/
and login.Option 1
console
enter:You'll see the response returned there.
Option 2
console
enterwpApiSettings.nonce
. You should see a valid nonce returned as a string.http://localhost:8889/wp-json/__experimental/url-details/?url=https://wordpress.org&_wpnonce=%YOUR_NONCE_HERE%
- be sure to replace the nonce value with that copied from the previous step.wordpress.org
(ie:"Blog Tool, Publishing Platform, and CMS \u2014 WordPress.org"
).Types of changes
New feature (non-breaking change which adds functionality).
Checklist: