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

Image block lightbox: PHP notices when an image with lightbox is deleted #55347

Closed
afercia opened this issue Oct 13, 2023 · 1 comment · Fixed by #55370
Closed

Image block lightbox: PHP notices when an image with lightbox is deleted #55347

afercia opened this issue Oct 13, 2023 · 1 comment · Fixed by #55370
Labels
[Block] Image Affects the Image Block [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended

Comments

@afercia
Copy link
Contributor

afercia commented Oct 13, 2023

Description

Discovered because of an edge case scenario but can be reproduced with a normal flow users can take in the WP admin.

Here:

if ( isset( $block['attrs']['id'] ) ) {
$img_uploaded_src = wp_get_attachment_url( $block['attrs']['id'] );
$img_metadata = wp_get_attachment_metadata( $block['attrs']['id'] );
$img_width = $img_metadata['width'];
$img_height = $img_metadata['height'];
} else {

we are trying to access the width and height array offsets that may not exist. In fact, wp_get_attachment_metadata can return false instead of an array.

On my local development environment, I had to re-import all my attachments via the WordPress importer and the theme unit test data becasue attachments got deleted while running PHP unit tests. By reimporting them, the post attachments got a new ID. Their path in the upload directory was unchanged, because I previously uploaded them in this same month:
/wp-content/uploads/2023/10/

Testing a post with an image block that opens in the lightbox:

  • The image was still visible, both in the Editor and on the front end, because the image path was unchanged.
  • The image actual id was new, although it stayed unchanged in the block markuo.
  • As such, wp_get_attachment_metadata returned false, triggering the PHP notices.

A more common scenario that users can potentially trigger by just using the WP admin is when deelting an image from the Media Library. See testing instructione below.

Seems to me that as a good practice, we should always check if an array item is set before trying to access it. Even though it's only a PHP notice that would probably be not visible on production, the code should be more safeguarder.

Step-by-step reproduction instructions

  • Create a post.
  • Add an image from the Media Library and set it to open in the lightbox.
  • Publish and view the front end.
  • Observe everything works as expected.
  • Go to the Media Library.
  • Delete the image that you previously added in the post.
  • View the previous post in the front end, refresh the page if necessary.
  • The image will be 'broken', and that is expected.
  • However these PHP notices are not expected:
PHP Notice:  Trying to access array offset on value of type bool in /var/www/src/wp-content/plugins/gutenberg/build/block-library/blocks/image.php on line 174

PHP Notice:  Trying to access array offset on value of type bool in /var/www/src/wp-content/plugins/gutenberg/build/block-library/blocks/image.php on line 175

Aside:
when refreshing the post in the Editor, you will get the additional, usual, PHP warning Cannot modify header information - headers already sent

Screenshots, screen recording, code snippet

No response

Environment info

No response

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

@afercia afercia added [Type] Bug An existing feature does not function as intended [Block] Image Affects the Image Block Backport to WP Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta labels Oct 13, 2023
@afercia afercia changed the title Image block lighbox: PHP notices when an image with lighbox is deleted Image block lightbox: PHP notices when an image with lightbox is deleted Oct 13, 2023
@afercia
Copy link
Contributor Author

afercia commented Oct 13, 2023

Given this bug is already in Core trunk, this should be fixed before the 6.4 release.

@Mamaduka Mamaduka removed the Backport to WP Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Oct 13, 2023
@github-actions github-actions bot added the [Status] In Progress Tracking issues with work in progress label Oct 15, 2023
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 [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants