HTML API: Fix missing null-check in wp_kses_hair() refactor.

[dmsnell]

Status

• Add test case to cover/catch this.

Description

Trac ticket: Core-63724

When no attributes are present, wp_kses_hair() should return an empty array, but when the refactor was merged, the code assumed there would be attributes.

An alternative fix is to use null-coalescing to iterate over an empty array. This would produce a marginally smaller function and read slightly more cleanly, but there’s no need to enter the foreach loop when it’s known in advance that there’s nothing over which to iterate.

Developed in: #10758
Discussed in: https://core.trac.wordpress.org/ticket/63724

Follow-up to: [61467]
Props: dd32, dmsnell.
See: Core-63724

[github-actions]

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

Core Committers: Use this line as a base for the props when committing in SVN:

Props dmsnell, jonsurrell.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[github-actions]

Test using WordPress Playground

The changes in this pull request can previewed and tested using a WordPress Playground instance.

WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Some things to be aware of

• The Plugin and Theme Directories cannot be accessed within Playground.
• All changes will be lost when closing a tab with a Playground instance.
• All changes will be lost when refreshing the page.
• A fresh instance is created each time the link below is clicked.
• Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
  it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

[sirreal]

This looks good, I prefer this to the null-coalescing alternative.

The early return could be moved before $syntax_characters is defined, but I'll leave that up to you.

[dmsnell]

Thanks @sirreal

The early return could be moved before $syntax_characters is defined, but I'll leave that up to you.

great point. no need to create that local var if we know we won’t need it.

[dmsnell]

Merged in [61499]
16aa10f

[dd32]

@dmsnell Slight nitpick, might be better to use empty() instead for cases like this, as it's more explicitly saying "If there's no matches.."

Using isset() works here obviously since it returns null (weirdly.. an empty array would be more WordPressy) but to a casual reader, It's set on the previous line, wtf?

[dmsnell]

interesting thought @dd32 — this is the more specific test.

we have avoided empty() in the HTML API code due to its ambiguity and ease of misuse (where checking for empty arrays we have tried to use 0 === count()). I would welcome further thought on this matter!

in this case it would have a similar effect: if the array is empty the loop will be a no-op. I guess it makes sense here to abort early just as in the null case 🤷‍♂️ (at the cost of then trying to figure out what the intent of the empty() was).

[dd32]

Yeah I figured y'all were avoiding empty() and using nully values in the HTML API, which admittedly is a very different structure than anything historically in core (Not necessarily a bad thing)

I just err on the side of less strict when not needed. It doesn't matter in this case if it's null, array(), or 0 all are not something you'd need to send into the foreach.
That's not the modern way of development it seems though; to be super-strict when never even needed 🤷

where checking for empty arrays we have tried to use 0 === count()

Which is fine when you're being strict and know that it's an array, but historically we've also had things that return false|array which yeah, annoying in old-code :)

I don't feel strongly here, I just felt the need to suggest that isset() feels weird. Effectively you're doing if ( is_null( $attribute_names ) ) instead.

[dmsnell]

That's not the modern way of development it seems though; to be super-strict when never even needed 🤷

Keep in mind that this entire subsystem arose in large part because of the decades of troubles caused by that looseness. We really do want people to trust the code and to have a reliable interface.

Personally I continue to see new unintended defects introduced through either empty() or if ( ! $implicitly_cast_to_bool ) so I find that it’s dangerous because of the frequency with which people are unaware of what exactly it’s doing. You actually had me wondering if we messed up the name of this already-menacingly-named function by not making it obvious enough that it can return null.

As you point out though, I think it makes sense here to abort if the array count is zero: why not? (I will toss in an update).

Effectively you're doing if ( is_null( $attribute_names ) ) instead.

I often forget which builtins are opcodes and which are actual function calls. I know that isset() is an opcode and does essentially one thing only: indicate if a variable is set and not null. Seems like in PHP 8.5 at least, null === $v and is_null( $v ) are identical. Interesting.