Improve parsing and retrieve additional data in REST url-details endpoint#31763
Merged
Improve parsing and retrieve additional data in REST url-details endpoint#31763
Conversation
6 tasks
|
Size Change: +234 kB (+14%) Total Size: 1.86 MB
ℹ️ View Unchanged
|
3 tasks
ef0fab7 to
791a621
Compare
getdave
commented
May 19, 2021
getdave
commented
May 19, 2021
getdave
commented
May 19, 2021
getdave
commented
May 19, 2021
3 tasks
Member
|
Code seems good to me. I'm -1 on spoofing a user agent though. If a site owner wants to block WordPress from making that API call, that should be their prerogative. |
Contributor
Author
@TimothyBJacobs Fair point. I was finding that a couple of sites seemed to be blocking requests so I assumed if the request looked more like a typical/common user agent then it would be ok. We can revert as the functionality can still be achieved via a filter if folks want to use my method in their own Plugin/code. |
Contributor
|
@getdave The above 3 suggestions take care of the remaining issues. Once committed, the PR should be ready for merge. |
Co-authored-by: Tonya Mork <hello@hellofromtonya.com>
Co-authored-by: Tonya Mork <hello@hellofromtonya.com>
Co-authored-by: Tonya Mork <hello@hellofromtonya.com>
Contributor
Author
|
@hellofromtonya Thanks for these. All committed 🥳 Let's get a final 👍 from @TimothyBJacobs so I can merge this. |
Contributor
|
Hey @TimothyBJacobs, |
Contributor
Author
|
@hellofromtonya One for a followup, but I've found an edge case where the icon is provided as a data URI: eg: <link href='data:image/png;base64,iVBORw0KGgo=' rel='icon' type='image/png'>We may need to account for situations such as these. |
Contributor
@getdave Aw yes, the data URL. Most browsers block these for top-level navigation due to security concerns. But it seems okay for a favicon. Testing in Firefox and Chrome, both rendered this example: when directly loading the URL into the browser. UPDATE: |
3 tasks
Contributor
|
@getdave Addressed the icon data URL in PR #32276 and cross-version PHP tested it https://3v4l.org/aaDc9. |
Member
|
Code is looking good to me. I haven't dived deep into the regexes, but the tests look good. The schema needs to be updated to account for the new data we are returning. |
TimothyBJacobs
approved these changes
May 28, 2021
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR is part of the effort to achieve the vision outlined in #31466. Namely the ability to show meta data about a remote URL when inserting a hyperlink using the block editor.
Following input from @hellofromtonya in #28791 (comment) this PR takes a regex based route to building on the original implementation from #18042 to add a better mechanism for parsing the information from the remote URL's response body HTML.
This helps to address feedback such as #27762 and also sets things up nicely for parsing additional detail from the remote URL (eg: favicon, Open Graph meta...etc).
This PR aims to:
It should also:
<head>tag.<title>tag (eg:<title data-rh="true">BBC - Home</title>).Note the Open Graph spec.
Closes #27762
Automated Testing Instructions
Run the test suite with
The tests themselves reside in
phpunit/class-wp-rest-url-details-controller-test.php.Manual Testing Instructions
npm run wp-env startto boot the local testing env.http://localhost:8889/wp-admin/and login.Option 1
consoleenter:...also try some non-English websites:
You should see a valid REST API response containing
wordpress.org(ie:"Blog Tool, Publishing Platform, and CMS \u2014 WordPress.org").Option 2
consoleenterwpApiSettings.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").