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

Improve logic in render_block_core_template_part. #50636

Merged

Conversation

spacedmonkey
Copy link
Member

What?

Improve logic in render_block_core_template_part. These improves include.

  • Skipping looking for files if slug is invalid.
  • Check to see if theme if the theme has a parent theme and not do anything.
  • Save repeated calls to get_stylesheet.

Why?

How?

Testing Instructions

Testing Instructions for Keyboard

Screenshots or screencast

Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

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

@spacedmonkey The logic here looks good, though it would be great to do a performance benchmark first to assess the impact.

It would be great to do two benchmarks, one for a regular block theme, and another one for a block theme with a child theme being used.

Based on the logic changes, it seems this change would be most beneficial for sites not using a child theme (which is probably the majority of sites).

@spacedmonkey
Copy link
Member Author

@felixarntz I applied these changes manaully to WordPress trunk and run benchmarks. 500 runs again, TT3 and TT3 child theme.

Trunk - TT3 PR - TT3 Trunk - TT3 Child PR - TT3 Child
Response Time (median) 109.4 105.55 114.65 109.1
wp-load-alloptions-query 0.64 0.64 0.64 0.64
wp-before-template (median) 52.23 53.1 57.54 56.82
wp-before-template-db-queries (median) 4.43 4.52 4.85 4.81
wp-template (median) 50.56 45.77 50.89 45.93
wp-total (median) 103.76 99.99 108.99 103.25
wp-template-db-queries (median) 2.58 2.03 2.55 1.96

This change seems to have a nice benefit for both types of site. 👍

@spacedmonkey spacedmonkey requested a review from felixarntz May 23, 2023 16:55
@swissspidy
Copy link
Member

I really wish Gutenberg would have PHP unit tests for all the block render callbacks 😞 That would make me much more confident in reviewing this change here. Maybe we could start adding some tests?

@spacedmonkey
Copy link
Member Author

Maybe we could start adding some tests?

@swissspidy I agree that the lack of a unit testing for callbacks is here is a real problem. But not one in the scope of this change. However we test these callbacks, how that is backported, it is another ticket.

I could maybe add tests once this is backported, that would be much easier.

Copy link
Member

@Mamaduka Mamaduka left a comment

Choose a reason for hiding this comment

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

I agree that unit testing is needed for essential logic in rendering callbacks like this.

It is also beneficial to provide testing instructions; they help reviewers and can later be adopted in tests. The original PR(#36910) was also missing the instructions, so apologies if I missed something.

Testing instructions

  • Create a child theme - wp scaffold child-theme tt3-child --parent_theme=twentytwentythree --theme_name="TT3 Child"
  • Override the footer template part using the legacy directory - tt3-child/block-template-parts/footer.html.
  • Confirm override works as expected - ✅
  • Rename the legacy template-parts directory - tt3-child/parts/footer.html.
  • Confirm override works as expected - ✅
  • Edit the template in Site Editor.
  • Confirm that editor content is rendered - ✅

Co-authored-by: George Mamadashvili <georgemamadashvili@gmail.com>
@github-actions
Copy link

Flaky tests detected in 9d55418.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5312394222
📝 Reported issues:

@spacedmonkey spacedmonkey merged commit 5d371bc into trunk Jun 19, 2023
@spacedmonkey spacedmonkey deleted the fix/improve-performance-render_block_core_template_part branch June 19, 2023 14:18
@spacedmonkey spacedmonkey added the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Jun 19, 2023
@github-actions github-actions bot added this to the Gutenberg 16.1 milestone Jun 19, 2023
@spacedmonkey spacedmonkey added [Package] Blocks /packages/blocks [Block] Template Part Affects the Template Parts Block labels Jun 19, 2023
@ramonjd ramonjd added the Backport to Gutenberg RC Pull request that needs to be backported to a Gutenberg release candidate (RC) label Jun 26, 2023
@tellthemachines tellthemachines removed Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta Backport to Gutenberg RC Pull request that needs to be backported to a Gutenberg release candidate (RC) labels Jun 27, 2023
sethrubenstein pushed a commit to pewresearch/gutenberg that referenced this pull request Jul 13, 2023
* Improve logic in `render_block_core_template_part`.

* Use a foreach loop.

* Apply suggestions from code review

Co-authored-by: George Mamadashvili <georgemamadashvili@gmail.com>

---------

Co-authored-by: George Mamadashvili <georgemamadashvili@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Template Part Affects the Template Parts Block [Package] Blocks /packages/blocks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants