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

Local Environment: Fix resolve for external hosts #73

Merged
merged 4 commits into from
May 9, 2024
Merged

Conversation

kozer
Copy link
Contributor

@kozer kozer commented May 1, 2024

Fixes https://github.com/Automattic/dotcom-forge/issues/6808
Fixes https://github.com/Automattic/dotcom-forge/issues/6738

Proposed Changes

This PR adds an extra mu-plugin to properly resolve domains for embedded urls.

Testing Instructions

Test 1

  1. Create a new site
  2. Open wp-admin
  3. Create a new page
  4. Add a new video block
  5. Try to embed a Youtube video. You should see something like below:
    2024-05-01T11:17:58,413230781+03:00
  6. Try to also embed a SoundCloud page.

Test 2

  1. Create a new site
  2. Open wp-admin
  3. Export a WPCOM site of yours. ( Ensure that it has media )
  4. Navigate to Tools -> Import.
  5. Import the exported XML file. Ensure that you have "ownload and import file attachments" selected.
  6. Ensure that after running import you don't see any errors.

Pre-merge Checklist

  • Have you checked for TypeScript, React or other console errors?

@kozer kozer requested a review from a team May 1, 2024 09:20
@kozer kozer self-assigned this May 1, 2024
@derekblank derekblank self-requested a review May 1, 2024 11:53
Copy link
Contributor

@derekblank derekblank left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirming from testing that the mu-plugin filter works as intended to embed media URIs for domains that are included in the $external_hosts array.

I left a comment regarding some other use cases for alternate YouTube URIs that can be shared that we may want to add to the $external_hosts array.

Also noting for comparison that when creating a new WordPress.com hosted site on the web (i.e., not a localhost site using Studio), there are other additional external hosts that are embeddable out-of-the-box (like Vimeo.com URIs, for example).

Approving as I don't consider either a blocker for this PR, but we could consider expanding the domains included in $external_hosts, too.

} );
add_filter( 'http_request_host_is_external', function( $allow, $host, $url ) {
$external_hosts = array(
'www.youtube.com',
Copy link
Contributor

@derekblank derekblank May 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noting that this URI will not embed YouTube URLs that do not include the www, and also the shortened domain youtu.be, which is the default URI given when sharing a YouTube video.

E.g., the following URIs would not be embeddable:

  • https://youtu.be/OmgooFTQhYQ?feature=shared
  • https://youtube.com/watch?v=FcTLMTyD2DU

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I refactored the function to handle those cases ( Actually in all of the examples you provided the host would end up being www.youtube.com, so it wouldn't be a problem, but we need that change the function after all, as it will easily resolve https://github.com/Automattic/dotcom-forge/issues/6738 )

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but we need that change the function after all, as it will easily resolve https://github.com/Automattic/dotcom-forge/issues/6738

Makes sense - I re-tested, and the refactored changes look good to me. Thanks for updating. 👍

@wojtekn
Copy link
Contributor

wojtekn commented May 2, 2024

Can we find a solution that doesn't involve adding hosts to the allowed list?

Related Playground issue: WordPress/wordpress-playground#400

@kozer
Copy link
Contributor Author

kozer commented May 2, 2024

Can we find a solution that doesn't involve adding hosts to the allowed list?

Related Playground issue: WordPress/wordpress-playground#400

I'm trying to find one, and this is why I didn't merge it so far. Wasn't familiar with that ticket, so thanks.
I've already communicated with Adam, and I'm waiting for his response.
In the meantime, I'm investigating it, as this solution is not good enough to handle https://github.com/Automattic/dotcom-forge/issues/6738.

As this seems bigger than expected, I propose to merge it ( as it seems something that users will want to do sooner or later ), and tackle this more appropriately in the issue mentioned above ( which should do either way ).

Then, we can remove this in that ticket, or create another one to remove this file all the way ( As I think it will resolve redirected hosts as well ).
What do you think @wojtekn ?

@kozer
Copy link
Contributor Author

kozer commented May 2, 2024

Sorry for mentioning you @wojtekn , I didn't notice you were afk. I added a "Reviewer can merge" so you can do whatever you want ( I'm afk the next days ).

@kozer
Copy link
Contributor Author

kozer commented May 9, 2024

@wojtekn , after the discussions we had, I updated this PR, to always return true for http_request_host_is_external filter, to resolve both of the issues mentioned in the description.
Note that this is a temp fix, and the proper solution is to fix php-wasm. As said in our messages, I'm in discussions with Adam to resolve the issue there, but in the meantime, let's deploy this fix.

@kozer kozer changed the title Local Environment: Fix resolve for embeded videos Local Environment: Fix resolve for external hosts May 9, 2024
Copy link
Contributor

@wojtekn wojtekn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works as expected, thanks for the fix.

@kozer kozer merged commit 40dc97e into trunk May 9, 2024
12 checks passed
@kozer kozer deleted the fix/embeded_videos branch May 9, 2024 11:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants