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.

Sign up for GitHub

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

Jump to bottom

Bug: PHP notice when an image with lightbox is deleted #55370

Merged
merged 2 commits into from
Oct 16, 2023
Merged

Bug: PHP notice when an image with lightbox is deleted #55370

merged 2 commits into from
Oct 16, 2023

Conversation

kishanjasani
Copy link
Contributor

@kishanjasani kishanjasani commented Oct 15, 2023 •
edited
Loading

wp_get_attachment_metadata return false if it has no metadata related to attachment, So, I added a condition to check before extrtacting the value from array. it help fixes: #55347

Testing Instructions

  • Create a post or page.
  • Add an image from the Media Library and set it to Expand on Click: true.
  • 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 and PHP notice should not show.

@kishanjasani kishanjasani changed the title Fix PHP notice when an image with lightbox is deleted Fix: PHP notice when an image with lightbox is deleted Oct 15, 2023
@kishanjasani kishanjasani changed the title Fix: PHP notice when an image with lightbox is deleted Bug: PHP notice when an image with lightbox is deleted Oct 15, 2023
ramonjd
ramonjd reviewed Oct 16, 2023
View reviewed changes
packages/block-library/src/image/index.php Outdated
Comment on lines 175 to 178
if ( ! empty( $img_metadata ) && is_array( $img_metadata ) ) {
$img_width = $img_metadata['width'];
$img_height = $img_metadata['height'];
}
Copy link
Member

@ramonjd ramonjd Oct 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This could cause an error. Since img_width and img_height won't be set, $img_metadata won't pass the conditions.

Undefined variable $img_width in /var/www/html/wp-content/plugins/gutenberg/build/block-library/blocks/image.php on line 216 Warning: Undefined variable $img_height in /var/www/html/wp-content/plugins/gutenberg/build/block-library/blocks/image.php on line 217

Maybe we could provide a default value like the below else statement.

Suggested change
if ( ! empty( $img_metadata ) && is_array( $img_metadata ) ) {
$img_width = $img_metadata['width'];
$img_height = $img_metadata['height'];
}
$img_width = $img_metadata['width'] ?? 'none';
$img_height = $img_metadata['height'] ?? 'none';

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @ramonjd, Thanks for the feedback. I already moved $img_width = 'none'; and $img_height = 'none'; out side of if statement. So, It won't throw error. I already checked the condition you mentioned with updated code.

Let me know, If I still need to make changes.

Copy link
Contributor Author

@kishanjasani kishanjasani Oct 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

By the way, Liked your short and optimal solution. I have updated the PR.

Thanks @ramonjd

Copy link
Member

Choose a reason for hiding this comment

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

Thank you for working on this!

Looks good to me, but maybe @artemiomorales should run an eye past it just to be sure.

@ramonjd ramonjd added [Type] Bug An existing feature does not function as intended [Block] Image Affects the Image Block labels Oct 16, 2023
@mikachan mikachan added the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Oct 16, 2023
michalczaplinski
michalczaplinski approved these changes Oct 16, 2023
View reviewed changes
Copy link
Contributor

@michalczaplinski michalczaplinski left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

Thank you @kishanjasani

@michalczaplinski michalczaplinski merged commit d9a75d0 into WordPress:trunk Oct 16, 2023
@github-actions github-actions bot added this to the Gutenberg 16.9 milestone Oct 16, 2023
@afercia
Copy link
Contributor

afercia commented Oct 16, 2023

@mikachan I think the same change should be done for the lib/block-supports/behaviors.php file, just in case. Quick PR incoming.

@afercia afercia mentioned this pull request Oct 16, 2023
Merged
SiobhyB pushed a commit that referenced this pull request Oct 16, 2023
@kishanjasani
Bug: PHP notice when an image with lightbox is deleted (#55370)
ab7369e 
* Fix PHP notice when an image with lightbox is deleted

* Fix PHP notice when an image with lightbox is deleted
@SiobhyB
Copy link
Contributor

SiobhyB commented Oct 16, 2023

I just cherry-picked this PR to the 6.4-rc1-2 branch to get it included in the next release: ab7369e

@SiobhyB SiobhyB removed the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Oct 16, 2023
SiobhyB pushed a commit that referenced this pull request Oct 16, 2023
@mikachan @tellthemachines
@t-hamano @ciampo @spacedmonkey @andrewserong @kishanjasani
Cherry-picked commits for WordPress 6.4 RC1 v2 (#55348)
8e94be2 
* Add selector as id to layout style overrides. (#55291)

* Fix flickering when focusing on global style variations (#55267)

* ProgressBar: use text color to ensure enough contrast against background (#55285)

* Use text color at different opacities for track and indicator

* Add high contrast mode styles

* CHANGELOG
# Conflicts:
#	packages/components/CHANGELOG.md

* Remove empty attrs. (#54496)

* Remove empty attrs.

* Fix linter errors

---------

Co-authored-by: Sarah Norris <[email protected]>

* Add IS_GUTENBERG_PLUGIN flag to LastRevision (#55253)

* useBlockPreview: Try outputting EditorStyles to ensure local style overrides are rendered (#55288)

* useBlockPreview: Try alternative fix for displaying local style overrides

* Avoid duplicate styles, fix rendering issues in Safari

* Add more explanatory comments

* Remove additional check for styles within the block preview, as it is not needed since EditorStyles handles its own style overrides retrieval

* Bug: PHP notice when an image with lightbox is deleted (#55370)

* Fix PHP notice when an image with lightbox is deleted

* Fix PHP notice when an image with lightbox is deleted

---------

Co-authored-by: tellthemachines <[email protected]>
Co-authored-by: Aki Hamano <[email protected]>
Co-authored-by: Marco Ciampini <[email protected]>
Co-authored-by: Jonny Harris <[email protected]>
Co-authored-by: Andrew Serong <[email protected]>
Co-authored-by: Kishan Jasani <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ramonjd ramonjd ramonjd left review comments

@michalczaplinski michalczaplinski michalczaplinski approved these changes

@artemiomorales artemiomorales Awaiting requested review from artemiomorales artemiomorales is a code owner

Assignees
No one assigned
Labels
[Block] Image Affects the Image Block [Type] Bug An existing feature does not function as intended
Projects
None yet
Milestone
Gutenberg 16.9
Development

Successfully merging this pull request may close these issues.

Image block lightbox: PHP notices when an image with lightbox is deleted
6 participants
@kishanjasani @afercia @SiobhyB @michalczaplinski @ramonjd @mikachan