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

Parsing: Use full parser in do_blocks with nested block support #11141

Merged
merged 22 commits into from
Nov 10, 2018

Conversation

dmsnell
Copy link
Member

@dmsnell dmsnell commented Oct 27, 2018

Updates do_blocks() and gutenberg_render_block() so that we can support nested blocks inside of dynamic blocks. This replaces the use of the partial parser which extracts registered dynamic blocks with the full parser.

This change will allow dynamic blocks which contain nested blocks inside of them and it will pave the way for a filtering API to structurally process blocks.

The partial parser came about at a time before the default parser was written; it was faster than the spec parser and was a tradeoff to get dynamic blocks rendering. The default parser, however, has been fast enough for a while to run on page render and so this PR exists to finally get it into the pipeline.

Status

Iteration

The RecursiveIteratorIterator should provide us a way of performing the depth-first block traversal without risking a stack overflow. We keep rendered blocks on a stack and pop them off as we go.

Interestingly enough I did some testing and RecursiveIteratorIterator was a let-down. There's one important thing it does which is bypass the xdebug.max_nesting_level which limits recursion depth when xdebug is running; sadly it's much much slower than basic recursion.

With the RecursiveIteratorIterator we can continue to nest into blocks until we run out of memory but the runtime performance is exponential. With recursion we hit the same memory limit if xdebug is disabled but hit it usually around 100 if it is. That means that for massive posts we could run into unexpected failures when debugging if we go the recursion route.

What is the memory limit? In my testing back to PHP 5.6 on my local laptop it was at around 100,000 levels of nesting - this should be obviously sufficient - I can't imagine an editor showing that many levels of nesting. With the recursive version and a limit of 100 that's still almost equally viable as 100,000. I'd recommend we stick with this recursive approach then unless we get bug reports of failures when debugging, at which point we can discuss bringing back the RecursiveIteratorIterator version.

On the other hand I may simply have been writing the WP_Block_Tree_Iterator in a stupid way that was way too slow. It would be worth running an experiment where I make a custom tree iterator without using the SPL functions and see if the performance characteristics are similar.

Update

After running some tests I made ended up with a basic tree iterator and it hit hard performance issues long before the recursive version. I think that the memory churning of the stacks required to track nodes in the tree is far less efficient than using the built-in argument passing in functions and closures provided by PHP with the recursive method.

In other words, I am currently unable to come close to the recursive version in terms of performance. Does this matter with small nesting? Probably not. Should we then prefer the safe version that won't break with low max_nesting_depth? I don't know. Probably not since anything over 100 levels of nested blocks is somewhat of an insane construction.

Testing and Review

The patch needs testing, but this can be hard because we don't have too many deeply-nested blocks. It's best to try with things like columns.

👉 I would greatly appreciate some help adding the appropriate deprecation messages for get_dynamic_blocks_regex() and friends!

@dmsnell dmsnell added the [Feature] Parsing Related to efforts to improving the parsing of a string of data and converting it into a different f label Oct 27, 2018
@pento pento added this to the 4.2 milestone Oct 29, 2018
lib/blocks.php Outdated
@@ -164,11 +164,32 @@ function gutenberg_render_block( $block ) {
}

This comment was marked as outdated.

@georgestephanis
Copy link
Contributor

encapsulated ^^ into a pr, with unit tests:

#11227

@youknowriad
Copy link
Contributor

Moving out of 4.2 as we have to release soon.

@youknowriad youknowriad modified the milestones: 4.2, WordPress 5.0 Oct 30, 2018
@dmsnell dmsnell force-pushed the parser/add-markers branch 2 times, most recently from 35e4377 to bff5592 Compare October 30, 2018 15:37
@dmsnell dmsnell changed the title Parsing: Use full parser in do_blocks with nested block support WIP: Parsing: Use full parser in do_blocks with nested block support Oct 30, 2018
@gziolo gziolo modified the milestones: WordPress 5.0, 4.4 Nov 5, 2018
@dmsnell dmsnell force-pushed the parser/do-blocks-with-full-parse branch from a9d0ecc to 03ca73d Compare November 7, 2018 22:19
@dmsnell dmsnell changed the base branch from parser/add-markers to master November 7, 2018 22:19
@dmsnell dmsnell changed the title WIP: Parsing: Use full parser in do_blocks with nested block support Parsing: Use full parser in do_blocks with nested block support Nov 7, 2018
@dmsnell
Copy link
Member Author

dmsnell commented Nov 8, 2018

Previous changes on this branch before the rebase…
4a4feba

@dmsnell dmsnell force-pushed the parser/do-blocks-with-full-parse branch 2 times, most recently from 79e6ec5 to 16a4175 Compare November 8, 2018 05:18
@dmsnell
Copy link
Member Author

dmsnell commented Nov 8, 2018

@aduth if you look at the failing tests you can see that we're hitting trailing-newline issues because in the existing do_blocks() we strip [\r\n] after a block's closing comments but here we're not.

can you help me understand what our desired end-product is so I can implement it? or, if it doesn't matter, should I update the test fixtures?

if we look at the parser output we can see that these newlines end up as freeform blocks whose contents are exactly \n\n. from a WordPress standpoint I guess that could risk introducing autop paragraphs? from an HTML standpoint it's just extra meaningless whitespace.

@dmsnell dmsnell force-pushed the parser/do-blocks-with-full-parse branch 4 times, most recently from aba181b to 6d713cd Compare November 9, 2018 16:14
@dmsnell dmsnell force-pushed the parser/do-blocks-with-full-parse branch from 8e2e243 to 7309d7f Compare November 10, 2018 02:17
@pento
Copy link
Member

pento commented Nov 10, 2018

get_dynamic_blocks_regex() hasn't been merged to Core, but it's still being used by strip_dynamic_blocks().

Upon reflection, I don't think strip_dynamic_blocks() was the right solution for the problem (#7268) that it was originally intended to solve. Something more like what @chrisvanpatten described would probably give us results more in line with how wp_trim_excerpt() is intended to behave.

I'll follow up with a PR after this one is merged to explore that.

Copy link
Member

@pento pento left a comment

Choose a reason for hiding this comment

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

👍🏻 Lovely, than you for wrangling this, @dmsnell!

@dmsnell dmsnell merged commit 283193f into master Nov 10, 2018
@aduth aduth deleted the parser/do-blocks-with-full-parse branch January 25, 2019 21:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Parsing Related to efforts to improving the parsing of a string of data and converting it into a different f
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants