strip whitespaces in render_block_core_cover

[navigatrum]

What?

Follows on from #39658 , #39658 (comment)
Remove whitespaces from html tags generated by render_block_core_covermarkup before injecting the featured image.

Why?

The fetaured image isn't injected from a template file beacuse of white spaces between html tags (#39658 (comment))

[github-actions]

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @navigatrum! In case you missed it, we'd love to have you join us in our Slack community, where we hold regularly weekly meetings open to anyone to coordinate with each other.

If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information.

[draganescu]

Thanks for taking care of this bug! :) I assumed the markup in the block's save function is fixed. I'd make the regexp more specific as ideally in the future we'd want to not need it at all so not encouraging reliance on it would be good.

@dmsnell this is spot on about our conversation around hand editing block markup and the assumptions we can make around that. It didn't break the editor in this case but it did break the expected idea that the final output is decided on save.

[dmsnell]

It didn't break the editor in this case but it did break the expected idea that the final output is decided on save.

Are you talking about the rendered page @draganescu? We shouldn't expect that the serialized HTML produced by Gutenberg and stored in post_content is the HTML someone will retrieve when reading a page online. Server-side processing is fundamental to how WordPress and Gutenberg interact.

We want to remember that the stored HTML is a serialized version of the document, but that Gutenberg posts themselves are not inherently HTML and the HTML they generate is both a stored serialization of those posts and a mechanism to provide more graceful fallback for preserving content when the supporting systems (like WordPress) are unavailable.

[ndiego]

Hi all, is this still something we are trying to get into 6.0, or can we push it to 6.0.1? Just let me know and I will update the project boards accordingly.

[ndiego]

Given that we are hours from 6.0 RC3, I am going to bump this to the 6.0.1 Project Board.

[dmsnell]

At first when I saw this I thought there was a very specific reason we needed the whitespace to be gone, but after looking a bit deeper it looks like we really just need to ensure that we can inject the image into the template.

Did you consider doing real parsing of the HTML in order to inject the image instead of relying on this considerably-more-frail text-parsing? I'm not sure what exactly your needs are, but I find replace( </span><div>, </span><img><div> ) to be incredibly confusing as a reader: what's the expected markup? what's the goal of the change? why these unlabeled tags? In fact, I don't really know how to review this because of that confusion. I'm going to assume we want to insert the featured image right before the wp-block-cover__inner-container <div> opens?

Parsing HTML

function render_block_core_cover( $attributes, $content ) {
	if ( 'image' !== $attributes['backgroundType'] || false === $attributes['useFeaturedImage'] ) {
		return $content;
	}

	if ( ! ( $attributes['hasParallax'] || $attributes['isRepeated'] ) ) {
		$attr = array(
			'class'           => 'wp-block-cover__image-background',
			'data-object-fit' => 'cover',
		);

		if ( isset( $attributes['focalPoint'] ) ) {
			$object_position              = round( $attributes['focalPoint']['x'] * 100 ) . '%' . ' ' . round( $attributes['focalPoint']['y'] * 100 ) . '%';
			$attr['data-object-position'] = $object_position;
			$attr['style']                = 'object-position: ' . $object_position;
		}

		$image = get_the_post_thumbnail( null, 'post-thumbnail', $attr );
		if ( ! $image ) {
			return $content;
		}

		$dom = new DOMDocument();
		$dom->loadHTML( $content );

		$inner_container = null;
		foreach ( $dom->getElementsByTagName('div') as $node ) {
			if ( 1 === preg_match( '/(^| )wp-block-cover__inner-container( |$)/', $node->getAttribute('class') ) ) {
				$inner_container = $node;
				break;
			}
		}

		if ( ! $inner_container ) {
			return $content;
		}

		$image_node = $dom->createDocumentFragment();
		$image_node->appendXML( $image );
		$inner_container->insertBefore( $image_node );

		$content = '';
		foreach ( $dom->documentElement->firstChild->childNodes as $node ) {
			$content .= $dom->saveHTML( $node ) . PHP_EOL;
		}
	} else {
		if ( in_the_loop() ) {
			update_post_thumbnail_cache();
		}
		$current_featured_image = get_the_post_thumbnail_url();
		if ( ! $current_featured_image ) {
			return $content;
		}

		$dom = new DOMDocument();
		$dom->loadHTML( $content );

		$container = null;
		foreach ( $dom->getElementsByTagName( 'div' ) as $node ) {
			if ( 1 === preg_match( '/(^| )wp-block-cover( |$)/', $node->getAttribute( 'class' ) ) ) {
				$container = $node;
				break;
			}
		}

		if ( ! $container ) {
			return $content;
		}

		$background_style = sprintf( 'background-image:url(%s)', $current_featured_image );
		$container->setAttribute('style', $container->getAttribute('style') . ' ' . $background_style );

		$content = '';
		foreach ( $dom->documentElement->firstChild->childNodes as $node ) {
			$content .= $dom->saveHTML( $node ) . PHP_EOL;
		}
	}

	return $content;
}

^^^ that's a little convoluted, but a few helper functions can make it read much smoother

Parsing HTML with helpers

function render_block_core_cover( $attributes, $content ) {
	if ( 'image' !== $attributes['backgroundType'] || false === $attributes['useFeaturedImage'] ) {
		return $content;
	}

	function html2dom( $html ) {
		$dom = new DOMDocument();
		$dom->loadHTML( $html );

		return $dom;
	}

	function dom2html( $dom ) {
		$html = '';
		foreach ( $dom->documentElement->firstChild->childNodes as $node ) {
			$html .= $dom->saveHTML( $node ) . PHP_EOL;
		}

		return $html;
	}

	function html2node( $dom, $html ) {
		$fragment = $dom->createDocumentFragment();
		$fragment->appendXML( $html );

		return $fragment;
	}

	function getTagWithClassname( $dom, $tag, $class ) {
		foreach ( $dom->getElementsByTagName( $tag ) as $element ) {
			if ( 1 === preg_match( '/(^| )' . preg_quote( $class, '/' ) . '( |$)/', $element->getAttribute('class') ) ) {
				return $element;
			}
		}

		return null;
	}

	if ( ! ( $attributes['hasParallax'] || $attributes['isRepeated'] ) ) {
		$attr = array(
			'class'           => 'wp-block-cover__image-background',
			'data-object-fit' => 'cover',
		);

		if ( isset( $attributes['focalPoint'] ) ) {
			$object_position              = round( $attributes['focalPoint']['x'] * 100 ) . '%' . ' ' . round( $attributes['focalPoint']['y'] * 100 ) . '%';
			$attr['data-object-position'] = $object_position;
			$attr['style']                = 'object-position: ' . $object_position;
		}

		$image = get_the_post_thumbnail( null, 'post-thumbnail', $attr );
		if ( ! $image ) {
			return $content;
		}

		$dom = html2dom( $content );
		$inner_container = getTagWithClassname( $dom, 'div', 'wp-block-cover__inner-container' );
		if ( ! $inner_container ) {
			return $content;
		}

		$inner_container->insertBefore( html2node( $dom, $image ) );
		return dom2html( $dom );
	}

	if ( in_the_loop() ) {
		update_post_thumbnail_cache();
	}
	$current_featured_image = get_the_post_thumbnail_url();
	if ( ! $current_featured_image ) {
		return $content;
	}

	$dom = html2dom( $content );
	$container = getTagWithClassname( $dom, 'div', 'wp-block-cover' );
	if ( ! $container ) {
		return $content;
	}

	$background_style = sprintf( 'background-image:url(%s)', $current_featured_image );
	$container->setAttribute( 'style', $container->getAttribute('style') . ' ' . $background_style );

	return dom2html( $dom );
}

Looks like this is now outdated based on a refactor of the function as well; we need to update the patch to account for that. I've made my suggestion here based on the more recent version of the file in trunk. Also I added a couple adjustments to handle cases when there's no thumbnail etc… Note that here we're appending the style to the style attribute instead of adding a new style attribute. According to my tests if we add multiple style attributes to an Element then only the last will apply; in other words, we don't merge styles, we only take the last available one.

The most relevant bit is this code here as it's the part that disambiguates the replacement operation (at least in my head it does a better job indicating what should be happening) and its resilient to whitespace, escaping, and formatting issues. As a bonus, it gives us the classnames as complete strings we can find with a basic search when we need to find and update this code:

$dom = html2dom( $content );
$inner_container = getTagWithClassname( $dom, 'div', 'wp-block-cover__inner-container' );
if ( ! $inner_container ) {
	return $content;
}

$inner_container->insertBefore( html2node( $dom, $image ) );
return dom2html( $dom );

[navigatrum]

@dmsnell I had considered the idea of Dom parsing, but I discarted it because because:

1. @draganescu didn't use it
2. DOMDocument is used in Wordpress very rarely compared to its potential fields of application, and not rarely with some disclaimers and after checks. See:

• we cannot universally count on DOMDocument being available

• if ( ! class_exists( 'DOMDocument', false ) )

• Handling HTML on the server
  As hinted in § Background, PHP can deal with HTML, but it’s a delicate operation. It’s no accident that most of WordPress has traditionally handled content as strings throughout its components: the tradeoff is less power in exchange for performance and safety. Thus, our utmost diligence is required when we set out to introduce a system that processes a new HTML document on every render_block. (from [Try: Reimplement Block Supports #26111])

[draganescu]

@dmsnell I think @navigatrum only applied a simple correction for code that was already merged, so their approach is valid in the context of how this already works.

While your suggestion to use DOMDocument to build block HTML on render is very good, it is something we as a project have not made use of so far (e.g. gutenberg_restore_group_inner_container, render_block_core_image, render_block_core_post_featured_image, render_block_core_template_part, render_block_core_site_logo).

From what I see, in general block HTML is generated with string manipulation. While what we do in the cover block, for the featured image case, is "injecting" html into whatever the block is saved as, is a bit different than general rendering, using DOMDocument to build a DOM object would be a completely new way of achieving a similar result.

The problem this particular PR tries to resolve is that manually editing block html in theme files may insert spurious whitespace. It is a general block problem that we happened to stumble upon here.

cc @hellofromtonya - should we try to move away from HTML string manipulation and favour DOM building? Are the old considerations that @navigatrum mentioned still a thing?

[hellofromtonya]

cc @hellofromtonya - should we try to move away from HTML string manipulation and favour DOM building? Are the old considerations that @navigatrum mentioned still a thing?

@draganescu No, I don't recommend building with PHP's DOM API. Why?

PHP's DOM API:

• the API can be disabled by web hosts
• requires the libxml extension (min version of 2.6.0) which may or may not be available on all web hosts
• struggles to handle malformed HTML. For example, a missing closing tag such as </div> or </head> can be impossible for it to detect and resolve
  • if malformed and it can't resolve it, then modifying nodes and eventually saving it out as HTML could mean broken layouts
• encoding (other than UTF-8) can be problematic

I'd love to see a DOM API land in Core. But, as noted above, when it can't work/run, then string fallbacks are still needed. Until the above are resolved, the use of PHP's DOM API is too limited.

[dmsnell]

:sigh:

earlier today I spent a while writing a large response and now I'm not seeing the comment I'm sure I posted. I'm going to try and write most of it again, but I'm sorry I won't be able to do it as eloquently or completely as I did before.

@hellofromtonya do we have any data on how many WordPress hosts have DOMDocument disabled? there's at least a couple un-guarded uses of it in Core already and I'm wondering if we have WordPress crashing in parts of the web we're unaware of.

@youknowriad @gziolo have you encountered this before? I've been considering more broad use of DOMDocument in Gutenberg to avoid a lot of common mistakes with over-simplifying HTML matching and parsing.

@navigatrum @draganescu it would be nice to consider the other suggestions I had, specifically in sharing more context for what we're changing and searching for. I'm trying to put my feet into the shoes of someone tasked with figuring out why certain markup is changing and broken and particularly the lack of anything in there that I can put into a simple string search and find this code is a big obstacle to debugging it.

alternatively we can make the RegExp pattern a little more verbose and get a lot more safety in the process. although this looks more complicated at the time of writing, I'm thinking it would be far more resilient in time and more helpful to the future given that it communicates from the code the specific thing it's looking for.

Fun fact: in PHP we are free to use other pattern delimiters which means we don't have to escape /

$inner_container_start = '~<div [^>]*\bclass=("|')((?!\1).)*\bwp-block-cover__inner-container\b((?!\1).)*\1( |>)~';

if ( 1 !== preg_match( $inner_container_start, $content, $matches, PREG_OFFSET_CAPTURE ) {
    // handle this, we can't find our div, maybe another plugin changed it
    return $content;
}

list( $fullmatch, $offset ) = $matches[0];
$content = substr( $content, 0, $offset ) . $image_tag . substr( $content, $offset );

this too can be split into functions that give better names to what we're looking for if it helps make the searching and replacement clearer.

function strpos_tag_with_class( $html, $class ) {
    $CLASS = preg_quote( $class, "~" );
    $pattern = "~<div [^>]*\bclass=(\"|')((?!\1).)*\b{$CLASS}\b((?!\1).)*\1( |>)~";

    if ( 1 !== preg_match( $pattern, $html, $matches, PREG_OFFSET_CAPTURE ) ) {
        return false;
    }

    list( $fullmatch, $offset ) = $matches[0];
    return $offset;
}

…

$image_at = strpos_tag_with_class( $content, "wp-block-cover__inner-container" );
if ( false === $image_at ) {
   return $content;
}

$before_image = substr( $content, 0, $image_at );
$after_image = substr( $content, $image_at );

return "{$before_image}{$image_tag}{$after_image}";

[youknowriad]

I think regex have been favored in general for performance reasons (but I may wrong), especially since all this logic renders in the frontend and we have a number of similar use-cases.

I wouldn't mind DOMDocument usage if it behaves better in terms of performance.

[hellofromtonya]

do we have any data on how many WordPress hosts have DOMDocument disabled?

I'm not sure that info is collected. @dd32 might know.

there's at least a couple un-guarded uses of it in Core already and I'm wondering if we have WordPress crashing in parts of the web we're unaware of.

@dmsnell besides build tooling and tests, can you point me to where DOMDocument is unguarded in WP Core? Thank you :)

Doing a search in the 6.0 released source (in build rather than develop), DOMDocument is used for (a) XML (e.g. SimplePie, WP_oEmbed, IIS web config files) and (b) detection - not to modify HTML nodes - (e.g. detect legacy in WP_Widget_Text, iis7_supports_permalinks() ). I'm not seeing unguarded usage.

[hellofromtonya]

Thinking more about this, I think I need to provide more clarity of where DOMDocument could be considered.

Possible use cases:

• XML
• Detection (with fallback)
• Possibly node selection (see ^* note below)

^*Possibly node selection: fallback would be needed if DOMDocument or libxml not available; and if selecting with translated content, then extensive testing and feedback especially with non-UTF-8 languages to ensure translated content renders correctly (encoding).

If there's zero risk of malformed HTML being passed into DOMDocument, e.g. the HTML structure is fully controlled within a block, and node selection does not include translatable content (meaning no encoding issues), then DOMDocument might be an appropriate tool. A fallback is needed for those users whose hosts do not have the libraries.

For these use cases, a Core DOM API might be handy to pull all the library checks, setups (such as turning on and off libxml errors, encoding, etc.), error handling, fallback detection, etc. into one place vs. distributing repeating/redundant code.

[navigatrum]

If there's zero risk of malformed HTML being passed into DOMDocument, e.g. the HTML is controlled within a block

@hellofromtonya Problem is when a block 'becomes' part of a theme template, HTML tends to become malformed. In my case I just copyed the code from the Gutenberg code Editor, and pasted it to my code editor. The auto formatter did the rest.

[draganescu]

@navigatrum just wanted to note that this tiny PR here takes so long not because it's wrong, but because it raises some possibly important conclusions around how we expect the system to be both resilient to hand coding markup and handling it's output consistently.

The replacement of spaces solution is good, but should we do that everywhere? Are there other blocks where a simple space in the template markup breaks the system? Hence the duration. Wanted to mention that and to thank you for the patience 🥇

[draganescu]

@navigatrum I think for this particular PR, we can just make it so that we don't introduce code that will stump us in the future via @dmsnell 's suggestions:

• let's add a comment explaining why the space removal is done
• let's make the expression more verbose, maybe include the block classname, so that it becomes more clear where in the markup the img is inserted and what is the expected shape of that markup.

What do y'all think?

[anton-vlasenko]

I used DOMDocument sometime in 2008, so I'm not an expert on the subject.
But I would rather not use it.
As noted above, WordPress sites will have to depend on libxml and the DOM library, and they are not available everywhere. Therefore, there must be a fallback mechanism (and if there is such a mechanism, then there is no need to depend on the DOMDocument class :).
The issue with handling invalid HTML code can be solved by using the LIBXML_HTML_NOIMPLIED option, but there is no guarantee that it will work correctly in all cases.
I favor using regular expressions and/or solving this issue in JavaScript (if possible).

[dmsnell]

I think regex have been favored in general for performance reasons (but I may wrong), especially since all this logic renders in the frontend and we have a number of similar use-cases.
I wouldn't mind DOMDocument usage if it behaves better in terms of performance.

@youknowriad this is the PHP DOMDocument we're talking about and all this is done in the render_block family of hooks.

@hellofromtonya upon further inspection I found that use in an expired build directory of Gutenberg but didn't notice it's stale because that directory is included in the .gitignore, so it looked to me like a real file. I'm not seeing unguarded uses after all

[azaozz]

Late to the party but may I add 2c? :)

DOMDoculemt sucks for parsing HTML. Enough said. It's great for generating (simple) XML. It works pretty well for parsing well-formed XML. But libxml cannot handle parsing HTML like a browser, and afaik it is not meant to. The main difference is that the browsers do a lot of "fixing" of badly formed or invalid HTML (including things like character encodings and entities, missing or extra closing tags, outdated/invalid stuff, and many more).

@dmsnell I think we had a similar conversation few years ago, tried using DOMDocument then too, and failed.

At the same time using Regex for "partially parsing" HTML sucks too. Core already has some examples there. If a tag has to be injected and that cannot be done in the browser, best would be to append (or prepend) the tag to the rest of the HTML in the block. That would probably look more like:

if ( '</div>' === substr( $block_html, -6 ) ) {
    $block_html = substr( $block_html, strlen( $block_html ) - 6 );
    $block_html = $block_html  . $my_img_tag . '</div>';
}

The rest (positioning, etc.) can be handled with CSS.

[dmsnell]

The replacement of spaces solution is good, but should we do that everywhere? Are there other blocks where a simple space in the template markup breaks the system?

Overall I think this is a disturbing indicator and ultimately falls back to the use of string-processing for HTML content. No whitespace changes should break HTML (apart from the fact that a leading newline/space after the opening of a tag inserts a single whitespace on render).

If we get to the point where we find that whitespace changes did break our code a far more reliable fix would be to update our code that depends on that whitespace sensitivity. Gutenberg itself is liberal in reformatting HTML so a quick hack here and there to force a specific HTML formatting so that processing stages later down the line don't crack is going to build a mess -quickly-, not to mention the readability issues when finding code like replace( '~</span>\s+<div~', '</span><div' ) (which is incredibly confusing and somewhat frustrating to come across).

I'd rather see us frame this as "the replacement of spaces solution works in this particular situation but we should try to eliminate the need for such a workaround that we'll soon regret."

Realistically this isn't the place or PR to demand that everything properly parse HTML (whether through DOMDocument() or through very complicated recursive PCRE patterns - yes PCRE can adequately parse HTML because they are more powerful than basic "regular expressions"). Still, we can get a lot better practically speaking and not have to come back here repeatedly as this crops up again.

In other words, the problem isn't the space before the <div> - the problem is that we're using too much of an over-simplified operation to inject the image tag and if we improve our matching code just a little bit we won't even have to talk about the whitespace 😄

[azaozz]

I think this is a disturbing indicator and ultimately falls back to the use of string-processing for HTML content.

Yeah, completely agree. String processing of HTML is best avoided at all costs. It may look simple and bulletproof but always has "strings attached" (pun intended) :)

[dmsnell]

@azaozz I missed your comment while I was writing mine.

I think we had a similar conversation few years ago, tried using DOMDocument then too, and failed.

sounds familiar: image source sets? I was hoping things have changed since then.

At the same time using Regex for "partially parsing" HTML sucks too.

Not sure I fully understand what you're saying, but I believe the code snippet you wrote is roughly equivalent to the one I wrote, and the one I wrote I think has the same failure modes, where this HTML appears inside a CDATA section. given that I couldn't figure out what the intentional change was, I assumed it was specific to the <div> with the particular class name so wanted to match on that. of note, I think we're injecting the image tag before the <div> starts, not at the end of it.

[azaozz]

sounds familiar: image source sets?

Yep, think so! :)

the code snippet you wrote is roughly equivalent to the one I wrote

Yes, it is. Was just trying to point out that it would be best to append (or prepend) an img tag at the end (or the beginning) of the block's HTML, not inject it in the middle. Then it would be a simple string concatenation, won't need regex, etc. Everything else can (most likely) be handled with CSS.

[draganescu]

It seems we can proceed here with a regexp and we have two suggestions from @dmsnell 👍🏻

1. ~<div [^>]*\bclass=(\"|')((?!\1).)*\b{$CLASS}\b((?!\1).)*\1( |>)~ or, more simply,
2. ~<div [^>]*\bclass=([\"'])[^\"']*[\"' ]wp-block-cover__inner-container[\"' ][^\"']*\1~

and the one from @navigatrum :

3. '/<div\b[^>]+wp-block-cover__inner-container[\s|"][^>]*>/U'

... which are very similar.

I think (1) is hard to follow, (2) and (3) are better, with (2) being even safer with the match on the "class" attribute name.

@navigatrum let's tweak the regex one last time and then this will land 👏🏻 You still up for it?

[dmsnell]

Pending the tests passing and this working as we expect it to, I approve 👍

Thanks for persevering with this!

[scruffian]

@navigatrum there's a PHP linting error on this, are you able to address it? Thanks!

[navigatrum]

@navigatrum there's a PHP linting error on this, are you able to address it? Thanks!

Wasn't it quotes around regex solved by admziel commit?

[scruffian]

Wasn't it quotes around regex solved by admziel commit?

There's another one with a missing ;

[scruffian]

Sorry I'm still seeing these errors:

FOUND 1 ERROR AND 4 WARNINGS AFFECTING 4 LINES
----------------------------------------------------------------------
 33 | WARNING | [ ] Unused variable $image.
    |         |     (VariableAnalysis.CodeAnalysis.VariableAnalysis.UnusedVariable)
 41 | WARNING | [ ] Unused variable $fullmatch.
    |         |     (VariableAnalysis.CodeAnalysis.VariableAnalysis.UnusedVariable)
 42 | WARNING | [x] Equals sign not aligned with surrounding
    |         |     assignments; expected 20 spaces but found 1
    |         |     space
    |         |     (Generic.Formatting.MultipleStatementAlignment.NotSameWarning)
 42 | WARNING | [ ] Variable $image_tag is undefined.
    |         |     (VariableAnalysis.CodeAnalysis.VariableAnalysis.UndefinedVariable)
 43 | ERROR   | [x] Blank line found after control structure
    |         |     (WordPress.WhiteSpace.ControlStructureSpacing.BlankLineAfterEnd)
----------------------------------------------------------------------

[adamziel]

The failing navigation E2E tests are flaky and unrelated to this PR. Let's ship it!

[github-actions]

Congratulations on your first merged pull request, @navigatrum! We'd like to credit you for your contribution in the post announcing the next WordPress release, but we can't find a WordPress.org profile associated with your GitHub account. When you have a moment, visit the following URL and click "link your GitHub account" under "GitHub Username" to link your accounts:

https://profiles.wordpress.org/me/profile/edit/

And if you don't have a WordPress.org account, you can create one on this page:

https://login.wordpress.org/register

Kudos!

[draganescu]

Thanks:

• @navigatrum on the work and effort to see this through
• @dmsnell for the great insights
• everyone else for the participation \o/
• @adamziel for the final button push 😁

[adamziel]

I just cherry-picked this PR to the wp/6.0 branch to get it included in the next release: c92a83a