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

Fix parsing in do_blocks() and rendering of blocks on frontend in the_content #1244

Merged
merged 6 commits into from Jun 20, 2017

Conversation

Projects
None yet
4 participants
@westonruter
Member

westonruter commented Jun 19, 2017

  • Change do_blocks to run as the_content filter at priority 9 instead of 10, to ensure it applies before wpautop. Blocks were getting processed after wpautop was being applied, causing problems with extra paragraphs being added erroneously (see below).
  • Fix matcher regular expression which was failing to select the contents and end of the block.
  • Improve regular expression readability since it was not broken up into its constituent parts, it lacked named capture groups and it needlessly escaped slashes. This will become irrelevant once #1152 is merged.
  • Blocks not registered on the server were not getting replaced, resulting in block delimiters leaking into the rendered frontend content.
  • Blocks were getting replaced with a global search/replace as opposed to replacing the blocks one-by-one for each one as it is being processed.
  • Adds missing tests for void blocks.

Before:

<p><div class="blocks-latest-posts">
	<ul>
		<li><a href='http://src.wordpress-develop.dev/2017/06/18/test-php-block-processing/'>Test PHP Block Processing</a></li>
<li><a href='http://src.wordpress-develop.dev/2017/06/15/test-latest-posts-block/'>Test latest posts block</a></li>

	</ul>
</div>
</p>
<p><!-- wp:core/text dropCap="false" --></p>
<p>Paragraph 1.</p>
<p><!-- /wp:core/text --></p>
<p><!-- wp:core/text dropCap="false" --></p>
<p>Paragraph 2.</p>
<p><!-- /wp:core/text --></p>

After:

<div class="blocks-latest-posts">
<ul>
<li><a href='http://src.wordpress-develop.dev/2017/06/18/test-php-block-processing/'>Test PHP Block Processing</a></li>
<li><a href='http://src.wordpress-develop.dev/2017/06/15/test-latest-posts-block/'>Test latest posts block</a></li>
</ul>
</div>
<p>Paragraph 1.</p>
<p>Paragraph 2.</p>
@westonruter

This comment has been minimized.

Show comment
Hide comment
@westonruter

westonruter Jun 19, 2017

Member

Note to self. After this I want to follow up with def41d5

Member

westonruter commented Jun 19, 2017

Note to self. After this I want to follow up with def41d5

@aduth

aduth approved these changes Jun 20, 2017

Couple remarks, but looks good.

Show outdated Hide outdated lib/blocks.php
// do nothing if the block is not registered.
$block_name = $matches['block_name'][ $index ][0];
$output = '';
if ( ! isset( $wp_registered_blocks[ $block_name ] ) ) {

This comment has been minimized.

@aduth

aduth Jun 20, 2017

Member

Not a guideline apparently, but in the case of conditions and ternaries, I find keeping positive form as the condition helps readability (reversing negative for else is awkward to follow). What do you think? In this case also allows you to collapse nested if:

if ( isset( $wp_registered_blocks[ $block_name ] ) ) {
	$block_attributes_string = $matches['attributes'][ $index ][0];
	$block_attributes = parse_block_attributes( $block_attributes_string );
	// Call the block's render function to generate the dynamic output.
	$output = call_user_func( $wp_registered_blocks[ $block_name ]['render'], $block_attributes );
} elseif ( isset( $matches['content'][ $index ][0] ) ) {
	$output = $matches['content'][ $index ][0];
}
@aduth

aduth Jun 20, 2017

Member

Not a guideline apparently, but in the case of conditions and ternaries, I find keeping positive form as the condition helps readability (reversing negative for else is awkward to follow). What do you think? In this case also allows you to collapse nested if:

if ( isset( $wp_registered_blocks[ $block_name ] ) ) {
	$block_attributes_string = $matches['attributes'][ $index ][0];
	$block_attributes = parse_block_attributes( $block_attributes_string );
	// Call the block's render function to generate the dynamic output.
	$output = call_user_func( $wp_registered_blocks[ $block_name ]['render'], $block_attributes );
} elseif ( isset( $matches['content'][ $index ][0] ) ) {
	$output = $matches['content'][ $index ][0];
}
preg_match_all( $matcher, $content, $matches, PREG_OFFSET_CAPTURE );
$new_content = $content;
$offset_differential = 0;

This comment has been minimized.

@aduth

aduth Jun 20, 2017

Member

Curious if it would be simpler to implement as while ( preg_match( $matcher, $content, $match ) ) {

Maybe a performance hit by repeatedly matching the same string.

@aduth

aduth Jun 20, 2017

Member

Curious if it would be simpler to implement as while ( preg_match( $matcher, $content, $match ) ) {

Maybe a performance hit by repeatedly matching the same string.

This comment has been minimized.

@westonruter

westonruter Jun 20, 2017

Member

@aduth good thought, but the problem is that preg_match doesn't accept an offset, so it would have to keep running the regex and search from the beginning of the string. So yeah, it would be less efficient.

Regardless, all of this regex logic will be eliminated once a PEG is implemented in PHP (#1152).

@westonruter

westonruter Jun 20, 2017

Member

@aduth good thought, but the problem is that preg_match doesn't accept an offset, so it would have to keep running the regex and search from the beginning of the string. So yeah, it would be less efficient.

Regardless, all of this regex logic will be eliminated once a PEG is implemented in PHP (#1152).

This comment has been minimized.

@aduth

aduth Jun 20, 2017

Member

preg_match does accept an offset:

http://php.net/manual/en/function.preg-match.php

@aduth

This comment has been minimized.

@westonruter

westonruter Jun 20, 2017

Member

Oh, TIL. In any case, re-using the one set of results from preg_match_all seems good.

@westonruter

westonruter Jun 20, 2017

Member

Oh, TIL. In any case, re-using the one set of results from preg_match_all seems good.

@westonruter westonruter merged commit 5eceb50 into master Jun 20, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@westonruter westonruter deleted the update/do-blocks-parsing-and-autop branch Jun 20, 2017

@mtias

This comment has been minimized.

Show comment
Hide comment
@mtias

mtias Jun 22, 2017

Contributor

Thanks for the fix @westonruter.

Contributor

mtias commented Jun 22, 2017

Thanks for the fix @westonruter.

@georgestephanis

This comment has been minimized.

Show comment
Hide comment
@georgestephanis

georgestephanis Sep 13, 2017

So this is kinda breaking other things in new and interesting ways.

By firing do_blocks() before wpautop() this is now causing new and unexpected problems.

For example, we're experimenting with converting some shortcodes into blocks -- but we're hitting inconsistent results because the existing shortcodes fire after wpautop, and blocks fire before -- so rando br tags are getting injected into the block's content after it's rendered. Which is certainly unexpected.

Talked this over with @matias at the GM, possibly worth looking at seeing if we can do a fix in wpautop to resolve the initial issue with wrapping p tags around html comments (which probably shouldn't happen anyways)

georgestephanis commented Sep 13, 2017

So this is kinda breaking other things in new and interesting ways.

By firing do_blocks() before wpautop() this is now causing new and unexpected problems.

For example, we're experimenting with converting some shortcodes into blocks -- but we're hitting inconsistent results because the existing shortcodes fire after wpautop, and blocks fire before -- so rando br tags are getting injected into the block's content after it's rendered. Which is certainly unexpected.

Talked this over with @matias at the GM, possibly worth looking at seeing if we can do a fix in wpautop to resolve the initial issue with wrapping p tags around html comments (which probably shouldn't happen anyways)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment