Make WordPress Core

Opened 6 months ago

Last modified 2 months ago

#59622 new defect (bug)

Block supports: Avoid warning for non string class attributes

Reported by: dmsnell's profile dmsnell Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 6.4
Component: Editor Keywords: has-patch has-unit-tests
Focuses: Cc:

Description

Block Supports: Avoid throwing warning when checking for class attribute as string.

When encountering HTML tags with boolean or missing tags, the get_attribute()
method in the HTML API returns true and null, respectively. If these returned
values are sent directly into string comparison functions then as of PHP 8.0 they
will throw E_DEPRECATED errors.

In this patch, block supports is enhanced to check that the class value is a
string before it performs string operations on it.

---

There is a small likelihood of any real damage in production here, as these are only deprecation notices and the logic in these cases isn't affected by the change. That is,
the intention of the code is maintained even when the attribute values aren't strings.

For background support there's a tiny but benign defect when the style attribute is
boolean. It will erroneously inject a ; at the beginning of the wrapper's new style
value. As this doesn't impact the rendering of the wrapper, it's not a true defect.

Further review belongs to block rendering code, but this will occur in the Gutenberg
repo since that PHP comes back into Core through the generated packages.

Change History (8)

This ticket was mentioned in PR #5486 on WordPress/wordpress-develop by @dmsnell.


6 months ago
#1

  • Keywords has-unit-tests added

Trac ticket: Core-59622

Block Supports: Avoid throwing warning when checking for class attribute as string.

When encountering HTML tags with boolean or missing tags, the get_attribute()
method in the HTML API returns true and null, respectively. If these returned
values are sent directly into string comparison functions then as of PHP 8.0 they
will throw E_DEPRECATED errors.

In this patch, block supports is enhanced to check that the class value is a
string before it performs string operations on it.

---

There is a small likelihood of any real damage in production here, as these are only deprecation notices and the logic in these cases isn't affected by the change. That is,
the intention of the code is maintained even when the attribute values aren't strings.

For background support there's a tiny but benign defect when the style attribute is
boolean. It will erroneously inject a ; at the beginning of the wrapper's new style
value. As this doesn't impact the rendering of the wrapper, it's not a true defect.

Further review belongs to block rendering code, but this will occur in the Gutenberg
repo since that PHP comes back into Core through the generated packages.

## Testing

There should be no functional or visual changes in this patch. Please consult blocks with layout and background support to verify that this hasn't adversely affected existing rendering. Verify that the test suite continues to pass.

@hellofromTonya commented on PR #5486:


6 months ago
#2

@dmsnell 10 Test_Block_Supports_Layout tests are failing from this change. Can you take a look please?

#3 @hellofromTonya
6 months ago

  • Milestone changed from Awaiting Review to 6.4

Pulling into the 6.4 milestone for awareness.

Would like to see this resolved to ship 6.4 without these errors. If it can be resolved before 6.4 RC1 tomorrow, then it could be considered for commit. However, currently the Test_Block_Supports_Layout automated tests are failing, which needs to be resolved.

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


6 months ago

@peterwilsoncc commented on PR #5486:


6 months ago
#5

Could the tests be failing because the markup is missing a closing div? <div><div class="wp-block-group"></div>

To an extent I'm asking the silly question because I'd sure feel silly if I didn't and it turned out to be the case ;)

@dmsnell commented on PR #5486:


6 months ago
#6

@peterwilsoncc no, and I wasn't sure what happened. I tried a few things that I thought should fix it, so I'm curious when my change is at fault or if the implicit behavior this removed is at fault, even though from the unit tests it looks like it should be fine.

#7 @hellofromTonya
6 months ago

  • Milestone changed from 6.4 to 6.5

The RC1 release party is about to start. Ran out of time to get this into the release. Moving to 6.5.

#8 @swissspidy
2 months ago

  • Milestone changed from 6.5 to Future Release

Moving out of the milestone as there hasn't been any traction at all and 6.5 Beta is approaching.

Note: See TracTickets for help on using tickets.