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

Temporarily broken images silently empties Image Block attributes #41161

Closed
eriktorsner opened this issue May 19, 2022 · 14 comments · Fixed by #41221
Closed

Temporarily broken images silently empties Image Block attributes #41161

eriktorsner opened this issue May 19, 2022 · 14 comments · Fixed by #41221
Assignees
Labels
[Block] Image Affects the Image Block [Feature] Media Anything that impacts the experience of managing media [Type] Bug An existing feature does not function as intended

Comments

@eriktorsner
Copy link

eriktorsner commented May 19, 2022

Description

In PR #35973 an error handler onImageError() was introduced in the Image Block with the intention to handle when images are removed from the Media Library. If the image is removed, the Image Block gets its id and URL attributes cleared out.

However, this behavior is also triggered when a local or remote image is just temporarily unavailable for any reason. For many users, this will be unwanted behavior.

There are many situations where an image can be unavailable for a short period of time without meaning that the image is broken forever and should be removed.

For remote images:

  1. Network issues including DNS problems
  2. Access control issues i.e due to config changes in a Cloud Front distribution
  3. Maintenance work
  4. Domain or SSL certificate problems.

For local images there are a few potential reasons as well:

  1. Images may be stored on a different file system that is temporarily unavailable
  2. Images may be intentionally removed from the local webserver by a plugin like WP Offload Media but with core issues like 54931 a local URL may be sent to the block editor.

Imagine a situation where a webmaster of a media-heavy site notes that the CDN is currently broken and then decides to edit the front page on the site to inform users of the problem. Well, now they have two problems.

Step-by-step reproduction instructions

On WordPress 6.0-RC2 (or any version of Gutenberg with the 35973 PR in place)

  1. Create a new page in the block editor
  2. Add an image block, click "Insert from URL" and use https://s.w.org/images/home/screen-themes.png
  3. Publish the page and exit the editor.
  4. View the page on the front end just to make sure the image is there.
  5. Now block access to the s.w.org domain using any suitable method on your computer. On Mac/Linux the easiest way is to add the line 127.0.0.1 s.w.org to /etc/hosts
  6. Verify on the front end that the image no longer shows up.
  7. Open the page in the editor. (note that due to browser caching, the image may still be visible. Clear the browser cache if needed)
  8. Note that the image block is now transformed to its original state, asking the user to upload an image or pick one from the Media Library.

Screenshots, screen recording, code snippet

No response

Environment info

  • WordPress 6.0-RC2 with Gutenberg 13?
  • Theme: twentytwentytwo
  • Server: nginx/1.21.6 using PHP 7.4.29
  • Db: MySQL 8.0.29

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

@Mamaduka Mamaduka added [Type] Bug An existing feature does not function as intended [Feature] Media Anything that impacts the experience of managing media [Block] Image Affects the Image Block labels May 19, 2022
@glendaviesnz
Copy link
Contributor

An explicit error message was added here to at least alert the user to the reason why the Image block reverted to the placeholder. The adding of this was related to instances where images had been permanently deleted, but maybe due to the case of the temporary connection errors you note the removing of the image details from the block needs to be revisited.

@danielbachhuber
Copy link
Member

However, this behavior is also triggered when a local or remote image is just temporarily unavailable for any reason. For many users, this will be unwanted behavior.

To build on this, one implication is that a user can "lose" images from their post if: 1) they edit a post with lots of images, 2) a few images fail to load for some temporary reason, 3) they save their post (thinking they're only applying the changes they made, but also saving the removal of the images).

@youknowriad
Copy link
Contributor

Can we instead of removing entirely the image, show a meaningful UI suggesting either reloading or removing?

@ramonjd @glendaviesnz should we consider reverting #35973 temporarily in 6.0 until we figure out a better approach?

@gziolo
Copy link
Member

gziolo commented May 20, 2022

should we consider reverting #35973 temporarily in 6.0 until we figure out a better approach?

I tried using the revert button but it didn't work. Still, it's probably the best way to move forward given that today is the last RC planned for WordPress 6.0.

@adamziel
Copy link
Contributor

Reverted in the wp/6.0 branch in 6446878

@gziolo
Copy link
Member

gziolo commented May 20, 2022

Let's work on a proper fix targeting WordPress 6.0.1.

@danielbachhuber
Copy link
Member

Can we instead of removing entirely the image, show a meaningful UI suggesting either reloading or removing?

Another option to consider (although I don't have a strong opinion it's a better one): if an image doesn't load, HEAD /wp-json/wp/v2/media/<image-id> and then silently remove the image only if the response is a 404.

This approach would achieve the intent of the original issue, and avoid the complexity of additional UI.

Reverted in the wp/6.0 branch in 6446878

So that I can set the correct expectations with the impacted customer, when do you think this change will be released in the Gutenberg plugin?


Huge thanks to @eriktorsner for the detailed initial report! That was 💯 🥇 .

@adamziel
Copy link
Contributor

So that I can set the correct expectations with the impacted customer, when do you think this change will be released in the Gutenberg plugin?

It is hard to reason about time estimates in an open-source project. As @gziolo mentioned, the idea is to fix this in WordPress 6.0.1 so probably no later than 6.0.1 is released. But when is that exactly? It's hard to tell, I don't think there's any fixed schedule for minor versions.

@gziolo
Copy link
Member

gziolo commented May 20, 2022

@ramonjd @glendaviesnz should we consider reverting #35973 temporarily in 6.0 until we figure out a better approach?

We removed that from WordPress 6.0. Should we also remove it from the Gutenberg plugin until the issue gets fixed or do you think the fix won't take long?

@ramonjd
Copy link
Member

ramonjd commented May 22, 2022

Thanks for catching this bug, and also for patching WordPress 6.0

We removed that from WordPress 6.0. Should we also remove it from the Gutenberg plugin until the issue gets fixed or do you think the fix won't take long?

I agree that we should remove the change from Gutenberg until we can fix the issue.

I'll revert the change so we can at least have parity, and will also take a look to see if there's an obvious way to improve the previous attempt.

Thanks a lot, folks!

ramonjd added a commit that referenced this issue May 23, 2022
And reverts #40982 since it's related to the original issue: #41161
@github-actions github-actions bot added the [Status] In Progress Tracking issues with work in progress label May 23, 2022
@ramonjd
Copy link
Member

ramonjd commented May 23, 2022

Hi folks!

Gutenberg revert PR

Moving a new investigation to a new PR

Thanks again for the excellent bug report and quick handling.

@gziolo
Copy link
Member

gziolo commented May 23, 2022

This issue is now resolved also in Gutenberg, the work continues in #41220. @ramonjd has also reopened old issues that no longer can be considered fixed as of the revert. Excellent work handling all that Ramon!

@danielbachhuber
Copy link
Member

Thanks for your work, @ramonjd !

@ramonjd
Copy link
Member

ramonjd commented Jun 23, 2022

I have an alternative approach ready over at #41220

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Image Affects the Image Block [Feature] Media Anything that impacts the experience of managing media [Type] Bug An existing feature does not function as intended
Projects
No open projects
Archived in project
Development

Successfully merging a pull request may close this issue.

9 participants