Update npm packages to latest versions for 6.4 RC1 #5468
Conversation
| * block content as-is. It may be that there's no actual image in the block | ||
| * or it could be that another plugin already modified this HTML. | ||
| */ | ||
| if ( false === stripos( $block_content, '<img' ) ) { |
There was a problem hiding this comment.
| if ( false === stripos( $block_content, '<img' ) ) { | |
| if ( ! str_contains( $block_content, '<img' ) ) { |
| @@ -16,11 +16,13 @@ | |||
| * @return string The block content with the data-id attribute added. | |||
| */ | |||
| function render_block_core_image( $attributes, $content, $block ) { | |||
| if ( false === stripos( $content, '<img' ) ) { | |||
There was a problem hiding this comment.
| if ( false === stripos( $content, '<img' ) ) { | |
| if ( ! str_contains( $content, '<img' ) ) { |
There was a problem hiding this comment.
FWIW, this code comes in via npm package sync from Gutenberg, so we'll need to fix it there.
There's another package sync planned for Monday; would it be okay to leave as-is until then if we prepare the change in GB and line it up for that package sync?
There was a problem hiding this comment.
Prepped a small PR at WordPress/gutenberg#55302, which can be included in the next package sync.
There was a problem hiding this comment.
(Also noting that strictly speaking, the code isn't 100% equivalent, since stripos is case-insensitive, whereas str_contains isn't. While block editor-generated code shouldn't contain non-lowercase HTML tags, we'd need to wrap $content in strtolower() to be 100% equivalent.)
There was a problem hiding this comment.
Given that the case-insensitivity, the requested changes may not be needed as they will require invoking strtolower() as @ockham noted. This can be discussed further in the small PR @mikachan opened. But it's not a blocker to this PR.
There was a problem hiding this comment.
for follow-up to complete the circle: this and the suggestion above it replacing stripos() with str_contains() introduce defects into the code.
thank you everyone who noticed.
There was a problem hiding this comment.
Thank you very much!
Approving; IMO, it's fine to address the str_contains suggestion in a follow-up.
There was a problem hiding this comment.
LGTM 👍
@spacedmonkey requested replacing the stripos() with str_contains(). There's a follow-up PR for that request WordPress/gutenberg#55302. The discussion can move over to that PR. Thus, IMO it should not be a blocker to committing this PR, as the updates are needed to unblock the next step of Core needed changes to resolve the regression issue.
IMO this is ready for commit.
|
Committed via https://core.trac.wordpress.org/changeset/56849. |
Trac ticket: https://core.trac.wordpress.org/ticket/59411
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.