Skip to content
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

Show rich previews for internal URLs in Link UI #32689

Closed
getdave opened this issue Jun 15, 2021 · 12 comments · Fixed by #33086
Closed

Show rich previews for internal URLs in Link UI #32689

getdave opened this issue Jun 15, 2021 · 12 comments · Fixed by #33086
Assignees
Labels
[Feature] Link Editing Link components (LinkControl, URLInput) and integrations (RichText link formatting) [Status] In Progress Tracking issues with work in progress

Comments

@getdave
Copy link
Contributor

getdave commented Jun 15, 2021

What problem does this address?

With #31464 we introduced "rich previews" for external URLs only.

It would be great if we could now do the same with internal URLs.

What is your proposed solution?

Update: after some discussion with @azaozz we've decided that Docker is blocking requests to itself when the requests come from within the container. This is probably because it isn't exposed somehow. Therefore we should be able to get previews for internal URLs working by using the existing method for fetching external URL data. It just requires to use a test setup which can make requests to itself (eg: MAMP...etc).

#32658 makes the APIs for fetching rich previews less specific to external URLs which should set us up nicely to fetch the data necessary for internal URLs.

However, the issue is that we don't always have a post ID to work with when using link control. Often we will have only a URL and we need to be able to use that to determine the post so that we can fetch the necessary data to populate the rich preview (eg: title, content, featured image...etc).

WP Core on the PHP side has [url_to_postid](https://developer.wordpress.org/reference/functions/url_to_postid/) which is ideal but we don't have an equivalent on the Gutenberg side.

Any ideas on how we can work around this welcome.

Alternatively we're going to need to make sure that LinkControl always provides a post ID back to the consumer if creating a link from an entity. Moreover, that consumer will have to store the postID alongside the URL and pass that back to <LinkControl> when rehydrating the link. Related issue #32282

@getdave getdave added the [Feature] Link Editing Link components (LinkControl, URLInput) and integrations (RichText link formatting) label Jun 15, 2021
@getdave
Copy link
Contributor Author

getdave commented Jun 15, 2021

Something like this might work but we're already doing x2 APIs requests and that's without fetching the feature image

if ( isInternal ) {
    return apiFetch( {
        path: `/wp/v2/search?url=${ urlOrId }`,
    } )
        .then( ( response ) => {
            return response[ 0 ] || {};
        } )
        .then( ( response ) => {
            return apiFetch( {
                path: `/wp/v2/${ response.subtype }s/${ response.id }&per_page=1`,
            } );
        } )
        .then( ( response ) => {
            return {
                title: response?.title?.rendered,
                icon: '',
                image: response?.featured_media,
                description: response?.excerpt?.rendered,
            };
        } );
}

@draganescu
Copy link
Contributor

How about making a new search handler for this specific thing? Look at the handlers for formats or taxonomies.

@getdave
Copy link
Contributor Author

getdave commented Jun 18, 2021

Note that I've tried allowing the url-details endpoint to request internal URLs. Unfortunately this only works for URLs that are accessible over HTTP and exposed to the public internet.

We could say localhost:8888 (or similar) is an exception and it's ok not to show previews to folks who are running WP on a non-web accessible URL.

However, the use case of this feature for previewing internal URLs is that it allows content authors to confidence check they have linked to the correct post. If we simply disable this then anyone who runs a WP site on a non-web accessible URL (and there are plenty!) won't get the benefit of this feature.

How about making a new search handler for this specific thing? Look at the handlers for formats or taxonomies.

I could write yet another REST API endpoint for this. I guess I wanted to check before I started overengineering solutions.

@draganescu
Copy link
Contributor

I guess I wanted to check before I started overengineering solutions.

By all means don't overengineer 😁 I don't know if that is a good idea.

However, the issue is that we don't always have a post ID to work with when using link control.

For posterity, why don't we always have an ID if the user picks a suggested post, category, taxonomy etc?

@azaozz
Copy link
Contributor

azaozz commented Jun 18, 2021

Thinking that previews for local URLs shouldn't be handled differently than remote URLs.

We could say localhost:8888 (or similar) is an exception and it's ok not to show previews

True, cURL requests to localhost:#### may fail depending on the server/devenv configuration. That may make testing a bit harder, but don't think these errors are a good reason to introduce a whole different way of handling of local URLs.

anyone who runs a WP site on a non-web accessible URL (and there are plenty!) won't get the benefit of this feature.

That's not exactly the case. WP sites on internal networks (not accessible from "outside") should still work as expected. The cases where cURL fails are mostly when WP is run in some sort of development environment. I think this is pretty similar to how WP "self-pings" are handled.

Perhaps showing a nice "Localhost cURL Error" message (targeted mostly at developers) would work well here.

@getdave
Copy link
Contributor Author

getdave commented Jun 21, 2021

That's good info. Thanks for helping me to think this through properly.

I completely agree that inventing a new way to fetch previews just for some minor edge cases isn't a good idea. Let's see if we can roll with the existing mechanic.

The only minor issue is, if the URL is not accessible (which is difficult to detect) and we allow an attempt at fetching data then we see a short loading state before the response fails.

We could simply blacklist all localhost URLs and have them abort early. Or we can simply accept the minor UX issue as an edge case.

Any suggestions welcome.

@draganescu
Copy link
Contributor

I say let the flicker there if it's only gonna happen on localhost.

@getdave
Copy link
Contributor Author

getdave commented Jun 22, 2021

@azaozz Weirdly if I run curl http://localhost:8888 from my terminal then it works. Why then would WordPress's HTTP API not work?

@azaozz
Copy link
Contributor

azaozz commented Jun 23, 2021

@getdave Good question :)

Guessing it may be something to do with Docker, perhaps (assuming you're running wpenv)? Looking around a bit, there are few reports about cURL needing some extra settings or steps to connect to the web server in a Docker image. Thinking it's worth investigating as fix(es) for wpenv.

@github-actions github-actions bot added the [Status] In Progress Tracking issues with work in progress label Jun 30, 2021
@getdave
Copy link
Contributor Author

getdave commented Jun 30, 2021

I say let the flicker there if it's only gonna happen on localhost.

How would you feel about ignoring any attempts to fetch URLs that have localhost? Mostly these won't be accessible via cURL.

@draganescu
Copy link
Contributor

It would be a weird experience while developing with WordPress not being able to setup a local environment that works just like the production one.

@getdave
Copy link
Contributor Author

getdave commented Jul 1, 2021

not being able to setup a local environment that works just like the production one.

Fair point, although the end result will be the same - no rich previews on localhost - which I why I was thinking it might be worth exploring.

Let's avoid exceptions and just let the environment do it's thing 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Link Editing Link components (LinkControl, URLInput) and integrations (RichText link formatting) [Status] In Progress Tracking issues with work in progress
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants