-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Port changes from core PR #26417
Port changes from core PR #26417
Conversation
Size Change: +459 B (0%) Total Size: 1.2 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a note, but LGTM!
WP_Block_Supports::$block_to_render = $parent_block; | ||
return $result; | ||
} else { | ||
return $block_render_callback( $attributes, $content ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor stylistic point: I'd prefer the early-return method (which we tend to prefer in JS too) over the if-else pair. And that way we also avoid double negatives like !== null
:
if ( null === $block ) {
return $block_render_callback( $attributes, $content );
}
$parent_block = …
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extra advantage for early-return – now the main logic branch is at the top execution level and the error handling is an obvious detail at a deeper level.
P.S. Sorry for the drive-by comment… :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nb: I still remember Else considered harmful. ;)
Co-authored-by: Jon Surrell <jon.surrell@automattic.com>
I want to coordinate changes from this PR WordPress/wordpress-develop#640 before merging this one. |
WordPress/wordpress-develop#640 was merged so I'm landing this one as well. Note that there's a failing end-to-end test. This is because CI is using latest wordpress-develop, so it'll be fixed as soon as it picks up the new patched version. |
…ss (#26417) This PR ports the changes from the core PR at WordPress/wordpress-develop#640 Co-authored-by: Jon Surrell <jon.surrell@automattic.com> Co-authored-by: Miguel Fonseca <miguelcsf@gmail.com>
This PR ports the changes done while preparing the block supports class to be ported to core. See WordPress/wordpress-develop#640
Specifically, this adds the following checks:
null
WP_Block. This is because there are some dead paths in core that are still used by tests, so we need to cover against that as well. See this comment for context.How to test
wp-block-social-links
is attached to the root element (ul) and not the children (li).