Make WordPress Core

Opened 2 years ago

Closed 2 years ago

#55241 closed defect (bug) (fixed)

Disabling "settings.color.duotone" within theme.json causes fatal error undefined variable $filter

Reported by: aliakseyenkaihar's profile aliakseyenkaihar Owned by: audrasjb's profile audrasjb
Milestone: 5.9.3 Priority: normal
Severity: normal Version: 5.9.1
Component: Themes Keywords: has-patch fixed-major
Focuses: Cc:

Description

With disabled "duotone" setting within theme.json file site crashes after 5.9.1 update

// theme.json
{
   "version": 2,
   "settings": {
        "color": {
	    "duotone": null
        }
    }
}

Error comes here – https://github.com/WordPress/WordPress/blob/master/wp-includes/class-wp-theme-json.php#L1597

As my duotone is empty, it skips definition of $filters variable

Fix: relocate definition of $filters = '' before empty( $node['color']['duotone'] ) check

Change History (20)

#1 @audrasjb
2 years ago

  • Milestone changed from Awaiting Review to 5.9.2

Hello @etilley welcome to WordPress Core Trac and thank you for reporting this issue,

Marking for 5.9.2 consideration.

Ping @mamaduka for information. For the moment I don't know if we need to fix this on Core or Gutenberg side (or both).

This ticket was mentioned in PR #2350 on WordPress/wordpress-develop by audrasjb.


2 years ago
#2

  • Keywords has-patch added

#3 @audrasjb
2 years ago

  • Owner set to audrasjb
  • Status changed from new to accepted

rafiahmedd commented on PR #2350:


2 years ago
#4

This PR looks good to me.

#5 @audrasjb
2 years ago

  • Keywords commit added

To reproduce the issue:

  • On Twenty Twenty-Two (or another block theme), use duotone: null color setting in theme.json file
  • The following warning is thrown: Notice: Undefined variable: filters in /wp-includes/class-wp-theme-json.php on line 1620

I can confirm the proposed patch fixes the issue.

Marking for commit.

#6 follow-up: @audrasjb
2 years ago

Introduced in [52768].

#7 follow-up: @audrasjb
2 years ago

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

In 52791:

Themes: Avoid undefined variable warning on get_svg_filters().

This change fixes an undefined variable warning thrown when duotone color setting was set to null in Block Themes theme.json` file.

Follow-up to [52768].

Props aliakseyenkaihar, audrasjb, rafiahmedd.
Fixes #55241.

#8 @audrasjb
2 years ago

  • Keywords fixed-major added; commit removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for backport to branch 5.9.

#10 in reply to: ↑ 6 @azouamauriac
2 years ago

Replying to audrasjb:

Introduced in [52768].

Hi sorry I don't really get it... what was introduced here?

#11 @audrasjb
2 years ago

@azouamauriac the issue.

#12 in reply to: ↑ 7 ; follow-ups: @azouamauriac
2 years ago

Replying to audrasjb:

In 52791:

Themes: Avoid undefined variable warning on get_svg_filters().

This change fixes an undefined variable warning thrown when duotone color setting was set to null in Block Themes theme.json` file.

Follow-up to [52768].

Props aliakseyenkaihar, audrasjb, rafiahmedd.
Fixes #55241.

The $filters was introduced here [52757] I think(please correct me if I'm wrong).

But I don't really get why it put there, however I think it should not be removed from there until we understand all consequences of move it.

What i'll suggest about the patch will be to leaving $filters at its place, and add the new initialization of this variable before the first loop for(what is already done).

#13 @audrasjb
2 years ago

#55234 was marked as a duplicate.

#14 in reply to: ↑ 12 @audrasjb
2 years ago

Replying to azouamauriac:

What i'll suggest about the patch will be to leaving $filters at its place, and add the new initialization of this variable before the first loop for(what is already done).

Correct me if I'm wrong, but this is exactly what [52791] aims to do :)

#15 in reply to: ↑ 12 @SergeyBiryukov
2 years ago

Replying to azouamauriac:

The $filters was introduced here [52757] I think(please correct me if I'm wrong).

You're right, that appears to be the initial changeset.

[52791] looks good to me as is, it just makes sure the variable is defined before the loop and not inside of it. This should work even if there are no items to iterate over.

#16 follow-up: @azouamauriac
2 years ago

@SergeyBiryukov @audrasjb let's take an example:

<?php
//1
$i = '';
$vals = array('a', 'b', 'c');
foreach ($vals as $key => $value) {
        $i = '';
        $i .= $value;
}
var_dump($i);


//2
$vals = array('a', 'b', 'c');
foreach ($vals as $key => $value) {
        $i = '';
        $i .= $value;
}
var_dump($i);


//3
$j = '';
$vals = array('a', 'b', 'c');
foreach ($vals as $key => $value) {
        $j .= $value;
}
var_dump($j);

1 and 2 do the same thing, but 1 looks better than 2, since $i was initialize before the loop, what 2 doesn't have.

But clearly, 3 will not give the same result with 1, right? that what i'm saying.

we don't need to remove(move) the $filters that inside the loop, otherwise we will change the default behavior, and may be break something.

I think the initialisation before the first loop is enough to fix this issue, but we don't need to remove the old initialisation inside the loop.

#17 @azouamauriac
2 years ago

#55246 was marked as a duplicate.

#18 in reply to: ↑ 16 @SergeyBiryukov
2 years ago

Replying to azouamauriac:

1 and 2 do the same thing, but 1 looks better than 2, since $i was initialize before the loop, what 2 doesn't have.

But clearly, 3 will not give the same result with 1, right? that what i'm saying.

Yes, you're right, the behavior is different.

Looking at the method description, it says: "Converts all filter (duotone) presets into SVGs". Before [52791], it looks like it would iterate over the whole $setting_nodes array, but then only return the last portion of presets. So I think the previous logic was incorrect, and the current code as of [52791] appears to be correct. That said, it would be great to add some unit tests for this :)

#19 @audrasjb
2 years ago

  • Milestone changed from 5.9.2 to 5.9.3

Moving to milestone 5.9.3 since we're about to release 5.9.2.

#20 @audrasjb
2 years ago

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

In 53009:

Themes: Avoid undefined variable warning on get_svg_filters().

This change fixes an undefined variable warning thrown when duotone color setting was set to null in Block Themes theme.json file.

Follow-up to [52768].

Props aliakseyenkaihar, audrasjb, rafiahmedd.
Merges [52791] to the 5.9 branch.
Fixes #55241.

Note: See TracTickets for help on using tickets.