QA: add missing early return statement

[jrfnl]

Why?

In contrast to the intention expressed in the comment in the code, the state was not being ignored as the function did not return within the if statement.

How?

As the docblock indicates that the function should return an array, I've chosen to return an empty array for this event, but I can imagine that throwing a RuntimeException or returning with a more specific error state in the array may be more appropriate.

Consider this PR as flagging up the code error, but the final implementation needs to be decided by people more familiar with the code than me.

[talldan]

It looks like this would change the way the parser works. Previously the token would be returned as a 'void-block' (which I imagine is a block type without any inner content), but now the token is ignored. I think it could result in content in a user's post not being displayed.

It seems like the intention before was to avoid being particularly strict, and that's what the comment means when it says 'ignore them since they don't hurt anything'.

I think a reasonable option would be to remove the if statement and replace it with an explanatory comment like this:

/*
 * A token could exist that matches this condition:
 *     $is_closer && ( $is_void || $has_attrs )
 *
 * This is ignored currently and treated as a void block, since it won't cause any issues.
 */

[jrfnl]

Definitely also an option 👍🏻

[anton-vlasenko]

Returning an empty array results in a PHP warning:

Warning: Undefined array key 0 in parser.php on line 256

Also, the rest of the document is not being parsed.

In my opinion, this is not the desired intention.
I'd display an error message instead (if WP_DEBUG is enabled).
This would allow to:
a) Let the user know that the block markup is broken;
b) Don't stop parsing; the rest of the blocks will be parsed.

[anton-vlasenko]

P.S.

I'd display an error message instead (if WP_DEBUG is enabled).

This could be explored in the follow-up PR.
For now, I'd fix the PHPCS error (per @talldan's suggestion above) and call it a day.

[jrfnl]

Well, I opened the PR explicitly as draft to get input on how this should be fixed from people more familiar with the code, see the PR description. So far, it doesn't look like there is consensus on what the fix should be.

[talldan]

I'm not convinced about throwing an error. This particular error would be triggered when there's invalid markup in user content, and in particular the user would probably be mangling block markup by hand to achieve such an error.

Throwing an error effectively invalidates the entire user's document making it unsalvageable to the average user (there isn't a single try/catch in the file, and from what I can tell it handles all looping itself in the parse function). There's a good chance much of the document is still valid, so my opinion is that it makes sense to try to continue trying to parse the rest of the document. I would think of it as similar situation to how a web browser will try to show a web page when there's incorrect markup.

What would an ideal fix look like? My thinking is that there would be some way to mark the particular token as invalid without invalidating the entire document, and also show these issues to users. This wouldn't be trivial, it'd involve developing a UI (both a block for visual editing mode, and a code highlighter for code editing mode) for displaying the errors.

What happens now in trunk isn't so bad, though it does make some assumptions in trying to fix the invalid content.

[jrfnl]

Rebased to resolve the conflict, though there is still no consensus on how to move this PR forward.

[Mamaduka]

Is there a consensus on this PR, or should we close it as stale?