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

[aliakseyenkaihar]

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

[audrasjb]

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).

Trac ticket: https://core.trac.wordpress.org/ticket/55241

This PR looks good to me.

[audrasjb]

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.

[audrasjb]

Introduced in [52768].

[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.

[audrasjb]

Reopening for backport to branch 5.9.

Committed in https://core.trac.wordpress.org/changeset/52791

[azouamauriac]

Replying to audrasjb:

Introduced in [52768].

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

[audrasjb]

@azouamauriac the issue.

[azouamauriac]

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).

[audrasjb]

#55234 was marked as a duplicate.

[audrasjb]

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 :)

[SergeyBiryukov]

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.

[azouamauriac]

@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.

[azouamauriac]

#55246 was marked as a duplicate.

[SergeyBiryukov]

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 :)

[audrasjb]

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

[audrasjb]

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.