Skip to content
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

Try: Block insertion via HTML processor #47089

Closed
wants to merge 4 commits into from

Conversation

joshuatf
Copy link
Contributor

@joshuatf joshuatf commented May 1, 2024

Changes proposed in this Pull Request:

Attempts to insert blocks by parsing the comment delimited format of blocks and allow direct insertion of content (HTML or blocks). This is an alternative to blocks and one that would need to be maintained separately so that requires a good amount of consideration, but I wanted to see what this might look like.

The three primary advantages to this are:

  1. Higher level of control and allowing insertion of HTML for 3PDs (3PDs seem to want this, but the downsides of this need to be debated since it could lead to inconsistent UI).
  2. Quick insertion of nested blocks that don't result in multiple tedious calls (e.g. column insertion).
  3. We can insert blocks on previously inserted blocks

This PR does not address:

  • Alias/variation names for blocks
  • Missing attributes and incomplete parsing of HTML
  • Metadata properties to ignore previously inserted blocks
Screenshot 2024-05-01 at 8 18 05 AM

How to test the changes in this Pull Request:

  1. Create a post with a couple blocks, including a heading
  2. Add the following snippet to a plugin:
add_filter( 'block_insertions', function( $content, $block_name, $position ) {
	$content_to_insert = <<<HTML
		<!-- wp:columns -->
		<div class="wp-block-columns">
			<!-- wp:column {"width":"50%"} -->
			<div class="wp-block-column" style="flex-basis:50%">
				<!-- wp:paragraph --><p>Test insertion column 1</p><!-- /wp:paragraph -->
			</div>
			<!-- /wp:column -->
			<!-- wp:column {"width":"50%"} -->
			<div class="wp-block-column" style="flex-basis:50%">
				<!-- wp:paragraph --><p>Test insertion column 2</p><!-- /wp:paragraph -->
			</div>
			<!-- /wp:column -->
		</div>
		<!-- /wp:columns -->
HTML;

	if ( $block_name === 'heading' && $position === 'after' ) {
		$content .= $content_to_insert;
	}
	return $content;
}, 10, 3 );
  1. See that the columns have been inserted in your post

Changelog entry

  • Automatically create a changelog entry from the details below.

Significance

  • Patch
  • Minor
  • Major

Type

  • Fix - Fixes an existing bug
  • Add - Adds functionality
  • Update - Update existing functionality
  • Dev - Development related task
  • Tweak - A minor adjustment to the codebase
  • Performance - Address performance issues
  • Enhancement - Improvement to existing functionality

Message

Comment

@joshuatf joshuatf requested review from tjcafferkey and a team May 1, 2024 12:30
@joshuatf joshuatf self-assigned this May 1, 2024
@github-actions github-actions bot added the plugin: woocommerce Issues related to the WooCommerce Core plugin. label May 1, 2024
Copy link
Contributor

github-actions bot commented May 1, 2024

Hi @tjcafferkey, @woocommerce/mothra

Apart from reviewing the code changes, please make sure to review the testing instructions as well.

You can follow this guide to find out what good testing instructions should look like:
https://github.com/woocommerce/woocommerce/wiki/Writing-high-quality-testing-instructions

@joshuatf
Copy link
Contributor Author

joshuatf commented May 1, 2024

I managed to get this working in block hooks as well. The caveat being that any nesting needs to happen all at once at the initial time of insertion, meaning one plugin could not extend another plugin's hooked tab or other field after its been inserted.

add_filter( 'hooked_block_types', function( $hooked_blocks, $position, $anchor_block, $context ) {
	if ( 
		'core/heading' === $anchor_block &&
		$position === 'after'
		) {
		$hooked_blocks[] = 'core/columns';
	}

	return $hooked_blocks;
}, 10, 4 );

add_filter( 'hooked_block_core/columns', function ( $parsed_hooked_block, $hooked_block_type, $relative_position, $parsed_anchor_block, $context ) {
	if (
		! isset( $parsed_anchor_block['blockName'] ) ||
		'core/heading' !== $parsed_anchor_block['blockName']
	) {
		return $parsed_hooked_block;
	}

	return array_merge(
		$parsed_hooked_block,
		array(
			'innerContent' => array(
				'<div class="wp-block-columns has-2-columns">',
				null,
				'',
				null,
				'</div>',
			),
			'innerBlocks' => array(
				array(
					'blockName' => 'core/column',
					'attrs' => array(),
					'innerBlocks' => array(
						array(
							'blockName' => 'core/paragraph',
							'attrs' => array(),
							'innerBlocks' => array(),
							'innerContent' => [ '<p>Test nested paragraph 1</p>' ],
						)
					),
					'innerContent' => [
						'<div class="wp-block-column">',
						null,
						'</div>',
					],
				),
				array(
					'blockName' => 'core/column',
					'attrs' => array(),
					'innerBlocks' => array(
						array(
							'blockName' => 'core/paragraph',
							'attrs' => array(),
							'innerBlocks' => array(),
							'innerContent' => [ '<p>Test nested paragraph 2</p>' ],
						)
					),
					'innerContent' => [
						'<div class="wp-block-column">',
						null,
						'</div>',
					],
				),
			)
		)
	);
}, 10, 5 );

@joshuatf
Copy link
Contributor Author

joshuatf commented May 1, 2024

We can actually let the block parser take care of most of that work, which does clean up the block hooks solution a bit. Obviously subsequent insertion on this hooked block is still not possible.

I'm also not 100% sure what the performance impact would be, if any, of parsing and serializing back and forth several times which is what needs to happen between block hooks and the below code.

add_filter( 'hooked_block_types', function( $hooked_blocks, $position, $anchor_block, $context ) {
	if ( 
		'core/heading' === $anchor_block &&
		$position === 'after'
		) {
		$hooked_blocks[] = 'core/columns';
	}

	return $hooked_blocks;
}, 10, 4 );

add_filter( 'hooked_block_core/columns', function ( $parsed_hooked_block, $hooked_block_type, $relative_position, $parsed_anchor_block, $context ) {
	if (
		! isset( $parsed_anchor_block['blockName'] ) ||
		'core/heading' !== $parsed_anchor_block['blockName']
	) {
		return $parsed_hooked_block;
	}

	$block_html = '<!-- wp:columns -->
		<div class="wp-block-columns">
			<!-- wp:column {"width":"50%"} -->
			<div class="wp-block-column" style="flex-basis:50%">
				<!-- wp:paragraph --><p>Test insertion column 1</p><!-- /wp:paragraph -->
			</div>
			<!-- /wp:column -->
			<!-- wp:column {"width":"50%"} -->
			<div class="wp-block-column" style="flex-basis:50%">
				<!-- wp:paragraph --><p>Test insertion column 2</p><!-- /wp:paragraph -->
			</div>
			<!-- /wp:column -->
		</div>
		<!-- /wp:columns -->';

	return parse_blocks( $block_html )[0];
}, 10, 5 );

@joshuatf
Copy link
Contributor Author

joshuatf commented May 1, 2024

Also added a naive function to handle adding the attributes back into tags in 5a93393. The following method will need to be copied to your WP_HTML_Tag_Processor class for this to work:

	/**
	 * Get the attributes for the current tag.
	 *
	 * @return array List of attributes.
	 */
	public function get_attributes() {
		return $this->attributes;
	}

This is a relatively simple change we could push upstream if we did want to move forward with a concept like this, but not sure if there are any repercussions to this or any errors that might result from recreating the HTML in this manner. cc @dmsnell

@dmsnell
Copy link

dmsnell commented May 1, 2024

@joshuatf I would discourage you from sharing $this->attributes as it's an internal data structure and does not represent the attributes on a tag (for instance, it doesn't take into account enqueued updates). also that will not return the decoded attribute values and there are just a number of reasons never to leak it.

instead, it would be better to rely on the public interface. if you want to clone and normalize a tag, it's pretty terse.

function normalize_tag() {
	if ( null === $this->get_tag() ) {
		return null;
	}

	$tag_name = strtolower( $this->get_tag() );

	if ( $this->is_tag_closer() ) {
		return "</{$tag_name}>";
	}

	$attributes = $this->get_attribute_names_with_prefix( '' );

	if ( null === $attributes ) {
		return "<{$tag_name}>";
	}

	$builder = new WP_HTML_Tag_Processor( "<{$tag_name}>" );
	foreach ( $attributes as $name ) {
		$builder->set_attribute( $name, $this->get_attribute( $name ) );
	}

	return $builder->get_updated_html();
}

@dmsnell
Copy link

dmsnell commented May 1, 2024

In WordPress/wordpress-develop#5705 and in WordPress/wordpress-develop#6381 I've been exploring lazy block parsers that follow a streaming model that's closer to the HTML API's design. We can reuse Core's methods for finding and parsing blocks, which would be good to do here rather than make simpler approximations, as those might lead to mistakes.

I'm not sure what's best here but I think it might work out to have multiple parsers that do different work, where the most basic, like in WordPress/wordpress-develop#5705, essentially segments a post by the block delimiters and maintains a stack of open blocks. That can be utilized for very performance search and modification operations where you don't need to know the global post structure or parse every block.

@joshuatf
Copy link
Contributor Author

joshuatf commented May 3, 2024

instead, it would be better to rely on the public interface. if you want to clone and normalize a tag, it's pretty terse.

Thanks, @dmsnell That's exactly what I was hoping for. I didn't realize we could use get_attribute_names_with_prefix with an empty string to get all the attributes. Had to add in $builder->next_tag() and then your

We can reuse Core's methods for finding and parsing blocks

I see that core's block parser primarily uses regex to find blocks and adds them to the open stack. IIRC you mentioned that there are potential issues with using regex to parse HTML, but maybe those are extreme edge cases when it comes to identifying blocks.

I'm not sure what's best here but I think it might work out to have multiple parsers that do different work, where the most basic

The concept of multiple parsers might be good. I'll post some performance stats below that I think demonstrate why it would be useful to have different tools for the job.

@joshuatf
Copy link
Contributor Author

joshuatf commented May 3, 2024

I ran some very quick and dirty performance tests to see how block hooks insertion (that utilizes core's parse_blocks and then re-serializes them) would compare against this PR's use of the HTML processor.

The below times are in microseconds and compare hooking/insertions on 1000 posts with either 1 or 10 insertions on each post. The block inserter in this PR supports recursion, so I ran some scenarios including that and excluding it for fair comparison. I also used the double parsing shown in #47089 (comment), which I ran a test without for comparison.

Notes Post Count Insertion Count Block Hooks Block Insertion
Recursion on block insertion 1000 1 0.14232301712036 0.37078905105591
No recursion 1000 1 0.13932108879089 0.24851703643799
Recursion on block insertion 1000 10 2.1987879276276 1.2394790649414
No double parsing, no recursion 1000 10 0.21408200263977 0.27611899375916

At first glance, the block insertion tool in this PR appears significantly slower, but most of that is due to recursion, which block hooks doesn't support currently. Removing that factor, the block inserter is still slightly slower; most of which appears from the need to rebuild the HTML document.

When we begin adding more blocks, the block inserter tool does a better job at scaling, but again that's primarily because we're parsing content twice (#47089 (comment)) where the inserter doesn't have this need.

Removing the recursion and double parsing, we see that the 2 tools are pretty close in performance, but each has scenarios where they do a better job, which might suggest something like @dmsnell's comment about having multiple parsers for different scenarios.

@joshuatf
Copy link
Contributor Author

With upcoming changes to the product editor form render, we no longer need block insertion at this level. Closing this PR.

@joshuatf joshuatf closed this Oct 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plugin: woocommerce Issues related to the WooCommerce Core plugin.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants