Make WordPress Core

Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#55161 closed defect (bug) (fixed)

Full Site Editing: PHP Warning with incomplete presets

Reported by: jeherve's profile jeherve Owned by: hellofromtonya's profile hellofromTonya
Milestone: 5.9.1 Priority: normal
Severity: normal Version:
Component: Editor Keywords: has-patch commit dev-reviewed fixed-major
Focuses: administration Cc:

Description

In some scenarios, especially on sites that started experimenting with Full Site Editing with the Gutenberg plugin when the feature was still under development, and then switched to WordPress 5.9, one may run into the following warning on the frontend, when using a block-based theme:

Warning: Undefined array key "fontFamily" in /wp-includes/class-wp-theme-json.php on line 1093

I believe the attached patch should get rid of the warning.

For reference, the warning was the following when using the Gutenberg plugin:

Warning: Undefined array key "fontFamily" in /wp-content/plugins/gutenberg/lib/compat/wordpress-5.9/class-wp-theme-json-gutenberg.php

Let me know if I should create this patch against https://github.com/WordPress/gutenberg/blob/efafb0a561c8ef2e171e6cb6430c3dbc00709bc0/lib/compat/wordpress-5.9/class-wp-theme-json-5-9.php instead.

Thank you!

Attachments (2)

55161.diff (1.0 KB) - added by jeherve 2 years ago.
Check if isset before to use.
55161-2.diff (984 bytes) - added by jeherve 2 years ago.
Simplify isset checks

Download all attachments as: .zip

Change History (13)

@jeherve
2 years ago

Check if isset before to use.

#1 follow-up: @swissspidy
2 years ago

  • Keywords needs-testing added
  • Milestone changed from Awaiting Review to Future Release

Instead of isset( $preset_metadata['value_key'] ) && isset( $preset[ $preset_metadata['value_key'] ] ) you can just combine the isset() calls like so:

isset( $preset_metadata['value_key'], $preset[ $preset_metadata['value_key'] ] )

#2 in reply to: ↑ 1 ; follow-up: @SergeyBiryukov
2 years ago

  • Milestone changed from Future Release to 5.9.1

Replying to swissspidy:

you can just combine the isset() calls like so:

isset( $preset_metadata['value_key'], $preset[ $preset_metadata['value_key'] ] )

Technically true, but that doesn't seem to be a common pattern in core, we seem to generally check multiple values separately. 55161.diff looks a bit more readable to me as is :)

#3 in reply to: ↑ 2 @jrf
2 years ago

Replying to SergeyBiryukov:

Replying to swissspidy:

you can just combine the isset() calls like so:

isset( $preset_metadata['value_key'], $preset[ $preset_metadata['value_key'] ] )

Technically true, but that doesn't seem to be a common pattern in core, we seem to generally check multiple values separately. 55161.diff looks a bit more readable to me as is :)

The fact that it's not used much in Core is no reason not to use it. I concur with @swissspidy that doing the check in one call to isset() is more efficient and is a good pattern to introduce in Core.

@jeherve
2 years ago

Simplify isset checks

#4 @hellofromTonya
2 years ago

  • Keywords commit added; needs-testing removed
  • Owner set to hellofromTonya
  • Status changed from new to reviewing

I agree with using one isset().

55161-2.diff LGTM. Marking for commit.

#5 @hellofromTonya
2 years ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 52763:

Editor: Adds an additional check to guard against incompete presets.

Adds an additional guard to ensure the value of $preset_metadata['value_key'] actually exists as a key in the $preset array. Fixes Warning: Undefined array key error.

Intentionally adds the check into the existing isset() as it's native to PHP, more efficient, and a good pattern.

Follow-up [52049].

Props jeherve, swissspidy, sergeybiryukov, jrf.
Fixes #55161.

#6 @hellofromTonya
2 years ago

  • Keywords dev-feedback fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for backport to 5.9 branch for 5.9.1 inclusion.

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.


2 years ago

#8 @SergeyBiryukov
2 years ago

  • Keywords dev-reviewed added; dev-feedback removed

[52763] looks good to backport.

#9 @hellofromTonya
2 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 52764:

Editor: Adds an additional check to guard against incompete presets.

Adds an additional guard to ensure the value of $preset_metadata['value_key'] actually exists as a key in the $preset array. Fixes Warning: Undefined array key error.

Intentionally adds the check into the existing isset() as it's native to PHP, more efficient, and a good pattern.

Follow-up [52049].

Props jeherve, swissspidy, sergeybiryukov, jrf.
Merges [52763] to the 5.9 branch.
Fixes #55161.

#10 follow-up: @hellofromTonya
2 years ago

Let me know if I should create this patch against https://github.com/WordPress/gutenberg/blob/efafb0a561c8ef2e171e6cb6430c3dbc00709bc0/lib/compat/wordpress-5.9/class-wp-theme-json-5-9.php instead.

It would be great to also apply this same change in the Gutenberg repo. @jeherve would you like to create a PR for it?

#11 in reply to: ↑ 10 @jeherve
2 years ago

Replying to hellofromTonya:

It would be great to also apply this same change in the Gutenberg repo. @jeherve would you like to create a PR for it?

Sure thing. Done so here:
https://github.com/WordPress/gutenberg/pull/38902

Note: See TracTickets for help on using tickets.