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

Photon: view context used in the block editor, causing media updates to use Photon URLs #24659

Closed
jeherve opened this issue Jun 7, 2022 · 5 comments · Fixed by #24769
Closed
Assignees
Labels
[Feature] Photon aka "Image CDN". Feature developed in the Image CDN package and shipped in multiple plugins [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack [Focus] Compatibility Ensuring our products play well with third-parties [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ [Pri] High [Type] Bug When a feature is broken and / or not performing as intended

Comments

@jeherve
Copy link
Member

jeherve commented Jun 7, 2022

Impacted plugin

Jetpack

Steps to Reproduce

This can only be reproduced when using the latest version of WordPress. version 6.0.

  1. Go to Posts > Add New
  2. Open your Network tools, and filter for media requests
  3. Add a new image block
  4. Upload or add an image from your media library
  5. Examine the media request that is triggered when the image is added to the block.

As you'll see:

  • The request uses the view context, which until WP 6.0 was only used on the frontend as far as I know. I don't know if that's a bug or something that was added on purpose. I could not find a related discussion in the Gutenberg repo about this.
  • Since it uses the view context, the different media URLs returned are processed by Photon. As a result, when resizing the image, you will be adding a Photon URL to your post content.

This causes an issue when resizing the image you've inserted, thus pulling a different media URL into the content.

You can see the issues in the following videos:

View context

https://videopress.com/v/QnOBCMWQ

Photon issue

https://videopress.com/v/8JCSALkF

In the past, we filtered out media requests that used the edit context to not use Photon: #10592

We could introduce a change and update the matching unit tests to also filter out view context requests:

diff --git a/projects/plugins/jetpack/class.photon.php b/projects/plugins/jetpack/class.photon.php
index ebdd49454d..f1144d5f06 100644
--- a/projects/plugins/jetpack/class.photon.php
+++ b/projects/plugins/jetpack/class.photon.php
@@ -1306,7 +1306,10 @@ class Jetpack_Photon {
 		if (
 			(
 				false !== strpos( $route, 'wp/v2/media' )
-				&& 'edit' === $request->get_param( 'context' )
+				&& (
+					'edit' === $request->get_param( 'context' )
+					|| 'view' === $request->get_param( 'context' )
+				)
 			)
 			|| false !== strpos( $route, 'wpcom/v2/external-media/copy' )
 		) {

That said, before we make such a change, I wanted to open a discussion to understand:

  • if that change happened for a reason
  • if it's a good idea to stop using Photon for all view requests; doing so would stop folks pulling content from the REST API from using Photon.

cc @nosilver4u who let me know about this context change; thank you again!

Other information

WordPress 6.0 Primary issue: #24082

@jeherve jeherve added [Type] Bug When a feature is broken and / or not performing as intended [Feature] Photon aka "Image CDN". Feature developed in the Image CDN package and shipped in multiple plugins [Pri] Normal [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack [Focus] Compatibility Ensuring our products play well with third-parties labels Jun 7, 2022
@github-actions github-actions bot added the [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ label Jun 7, 2022
@jeherve
Copy link
Member Author

jeherve commented Jun 7, 2022

@jsnajdr I was wondering if you had some insight on that WordPress 6.0 change?

@anomiex
Copy link
Contributor

anomiex commented Jun 7, 2022

A glance through the backtraces of the request and a little git archaeology suggests WordPress/gutenberg#39580 is the source of the change.

@jeherve
Copy link
Member Author

jeherve commented Jun 7, 2022

@anomiex Nice find, that is it indeed. The change does make sense in the context of the issue it solves, and that request is really only to view information and not edit media data. However, it does seem like it could create issues with third-party plugins like Jetpack.

I'm not sure how to best address this. I suppose Core could add an additional parameter that would inform about the page we're on, not just the API request context, but I don't know if that's something that would be worth implementing in Core. @Mamaduka I would be curious to have your take on this, since you worked on the original PR. Thank you!

@anomiex
Copy link
Contributor

anomiex commented Jun 7, 2022

I'm not sure how to best address this. I suppose Core could add an additional parameter that would inform about the page we're on, not just the API request context, but I don't know if that's something that would be worth implementing in Core. @Mamaduka I would be curious to have your take on this, since you worked on the original PR. Thank you!

To clarify, the goal here is to know whether the REST API request is for the editor, so Jetpack can avoid transforming the image URLs included in the response that may eventually make their way into the saved post content. It doesn't need to be a parameter, it could as well be an HTTP header or something.

Since the REST API request here goes via @wordpress/api-fetch, I wonder if there's a way we could register a global middleware that could add such a header?

@Mamaduka
Copy link

Hi, @anomiex, @jeherve

We had to update context to view for a few media blocks in the core to avoid 403s for users with lower caps. The view context tells REST API to return fields available for display and skip the capability check required for edit fields. In the case of these media blocks, this is the correct context for the fields we're consuming.

Unfortunately, I'm unfamiliar with how Photon integration works to help further with this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Photon aka "Image CDN". Feature developed in the Image CDN package and shipped in multiple plugins [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack [Focus] Compatibility Ensuring our products play well with third-parties [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ [Pri] High [Type] Bug When a feature is broken and / or not performing as intended
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants