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

Fix regression - Allow blocks with names to pass filtering, even if empty when rendered #105

Merged

Conversation

williamjulianvicary
Copy link
Contributor

Adds a fix for a regression caused by #59

Blocks may intentionally be empty rendered, and the properties passed to the frontend for rendering on the frontend the previous change in this file caused a regression which blocked those use cases, while adding support for Classic Editor blocks (which didn't have a name and were therefore filtered).

This PR allows both:

  • If blocks have a name, they pass the filter regardless.
  • Then the block is rendered and checked for empty()

As a side-effect, this should be more performant as it's avoiding a render.

Blocks may intentionally be empty rendered, and the properties passed to the frontend for rendering on the frontend the previous change in this file caused a regression which blocked those use cases.
@williamjulianvicary williamjulianvicary requested a review from a team as a code owner May 25, 2023 16:22
@changeset-bot
Copy link

changeset-bot bot commented May 25, 2023

🦋 Changeset detected

Latest commit: 07f8c89

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@wpengine/wp-graphql-content-blocks Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@justlevine justlevine mentioned this pull request May 26, 2023
@theodesp
Copy link
Member

Hey @williamjulianvicary since we allow empty blocks, should we just remove the whole filtering logic altogether?

// 1st Level filtering of blocks that are empty
		$parsed_blocks = array_filter(
			$parsed_blocks,
			function ( $parsed_block ) {
				// Strip empty comments and spaces
				$stripped = preg_replace( '/<!--(.*)-->/Uis', '', render_block( $parsed_block ) );
				return ! empty( trim( $stripped ?? '' ) );
			},
			ARRAY_FILTER_USE_BOTH
		);

@williamjulianvicary
Copy link
Contributor Author

I don't quite understand the implications of that yet, but I think this is tied to classic blocks. In the tests there are fake posts with content like:

  <!--  -->
                <!--  -->
                <!-- wp -->
                <!-- /wp -->

                <!-- wp: -->
                <!-- /wp: -->

My assumption was that the change was to support keeping classic blocks (which are not empty, but do not have a name), still strip the above empty-non-named blocks out of the CSS and my addition then allows blocks that are named but also empty.

Note: The logic I've added, is actually reverting part of the change that was added in the recent addition of Classic Block support, originally it only checked the name - this didn't work for Classic Editor blocks, but the change then caused a regression with named empty blocks - my change here is retaining the new logic for removing empty blocks but adding back in the retention of named blocks.

Make sense? TL;DR I think both pieces of logic still have a place, removal will break tests, and I'm not sure on the knock on effect of retaining those empty blocks on the graphql output without running some tests.

Copy link
Member

@theodesp theodesp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@theodesp theodesp merged commit 5765443 into wpengine:main May 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants