-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Navigation link: prime caches for all posts in menu items #40752
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@spacedmonkey Mostly looks good, just a few minor comments.
The higher-level feedback though is that I think we should add a test for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor docblock suggestions and some stricter assertions.
Co-authored-by: Colin Stewart <[email protected]>
@anton-vlasenko @draganescu @gziolo Any recommendation on this PR to improve the backport. The files in 6.1 compat file do not seem right to be but it was the only way to get the unit tests to run. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks sensible to me, I've added a note about a more complex test but it would be great to have the cache priming in even if the test waits a little longer.
@spacedmonkey when I checked in the build files ( /**
* Get post IDs from a navigation link block instance.
*
* @param WP_Block $block Instance of a block.
*
* @return array Array of post IDs.
*/
function gutenberg_gutenberg_block_core_navigation_get_post_ids_from_block( $block ) {
$post_ids = array();
if ( $block->inner_blocks ) {
$post_ids = gutenberg_block_core_navigation_get_post_ids( $block->inner_blocks );
}
if ( 'core/navigation-link' === $block->name || 'core/navigation-submenu' === $block->name ) {
if ( $block->attributes && isset( $block->attributes['kind'] ) && 'post-type' === $block->attributes['kind'] ) {
$post_ids[] = $block->attributes['id'];
}
}
return $post_ids;
}
/**
* Iterate through all inner blocks recursively and get navigation link block's post IDs.
*
* @param WP_Block_List $inner_blocks Block list class instance.
*
* @return array Array of post IDs.
*/
function gutenberg_block_core_navigation_get_post_ids( $inner_blocks ) {
$post_ids = array_map( 'gutenberg_block_core_navigation_get_post_ids_from_block', iterator_to_array( $inner_blocks ) );
return array_unique( array_merge( ...$post_ids ) );
} I have no idea where does the double |
@adamziel So what do you recommend here? Is this PR okay as it is? |
@spacedmonkey I'd like to see these functions bundled with the navigation block since they are only needed there. At the same time, the time before the RC2 is short and this looks like a bug in the build process. I am investigating whether this can be fixed quickly. |
@spacedmonkey got it! The matcher finds If you rename these functions in such a way that the name of the first is not a substring of a second, it should work. You may also need to call the I've filed an issue to keep track of this problem separately: #40938 |
@adamziel Thanks. I have pushed up a change. |
I cherry-picked this to wp/6.0 branch to be included in WordPress 6.0 RC2 later today 57e4435 |
* Prime posts in navigation menus. * Improve logic again. * Change variable name and function names. * Feedback and tweaks. * Add a unit test. * Fix lints. * Apply suggestions from code review Co-authored-by: Colin Stewart <[email protected]> * Move functions * Improve comment. * Add another unit test. Co-authored-by: Colin Stewart <[email protected]>
We will still need to add unit tests separately to WordPress core. |
What?
For menu items in the navigation, instead of loading each post one at a time, load all posts into memory by get all post ids and running them through the
_prime_post_caches
function.Fixes #40750.
Why?
This improves database performance.
How?
Testing Instructions
Screenshots or screencast