Make WordPress Core

Opened 8 months ago

Closed 8 months ago

Last modified 8 months 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 8 months ago.
Check if isset before to use.
55161-2.diff (984 bytes) - added by jeherve 8 months ago.
Simplify isset checks

Download all attachments as: .zip

Change History (13)

@jeherve
8 months ago

Check if isset before to use.

#1 follow-up: @swissspidy
8 months 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
8 months 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
8 months 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
8 months ago

Simplify isset checks

#4 @hellofromTonya
8 months 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
8 months 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
8 months 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.


8 months ago

#8 @SergeyBiryukov
8 months ago

  • Keywords dev-reviewed added; dev-feedback removed

[52763] looks good to backport.

#9 @hellofromTonya
8 months 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
8 months 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
8 months 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.