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.

Sign up for GitHub

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

Ensure that post thumbnail is cached in post template block. #40572

Merged
merged 4 commits into from
May 6, 2022
Merged

Ensure that post thumbnail is cached in post template block. #40572

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
27b625f
Ensure that post thumbnail is cached in post template block.
spacedmonkey Apr 24, 2022
d38e647
Check for inner blocks that use featured images.
spacedmonkey May 5, 2022
18a25c6
Default value.
spacedmonkey May 5, 2022
0a20bcb
Improve logic.
spacedmonkey May 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/block-library/src/post-template/index.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ function render_block_core_post_template( $attributes, $content, $block ) {
if ( ! $query->have_posts() ) {
return '';
}
update_post_thumbnail_cache( $query );
Copy link
Contributor

@ntsekouras ntsekouras Apr 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Since a Query Loop block might not contain the Post Featured Image block, wouldn't this result in extra db queries in that case? Can we incorporate this in the Post Featured Image block somehow and have some performance improvements?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fair point. I have moved the logic to the feature image block.

I tried to get the query as part of context, why I am not sure if this is even possible. My worry is the the feature image is used in a query block or other place, where the query is different than the main query on the page.

Any ideas @ntsekouras

Copy link
Contributor

Choose a reason for hiding this comment

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

👋 Post Featured Image knows if is in a Query Loop if the queryId prop is filled from its context. An example would be here, but we would need to check for queryId. Noting that the queryId could be zero.

I'm not really familiar with this optimization, but would it make sense in the block itself or is it more about passing a query?

Copy link
Member Author

Choose a reason for hiding this comment

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

The function update_post_thumbnail_cache accepts the query object as a param. When it does, all thumbnail caches are primed in one request. It uses the WP_Query object to get the posts and then loop the thumbnails.

I think we should pass the query in here, as a feature image block could be used in different query loops.

Copy link
Contributor

Choose a reason for hiding this comment

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

I took a closer look here and I think this could work well in Post Template block.

foreach ( $block->parsed_block['innerBlocks'] as $inner_block ) {
	if ( 'core/post-featured-image' === $inner_block['blockName'] ) {
		update_post_thumbnail_cache( $query );
		break;
	}
}

Can you think of any implications with this code @gziolo?

Copy link
Member

Choose a reason for hiding this comment

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

@ntsekouras, that would be similar to what @spacedmonkey proposes in #40752 for the Navigation block.

Anyway, it largely depends on whether the Post Featured Image is the immediate child in the Post Template or it can be included at any level of nesting. I'm not sure whether $block->parsed_block['innerBlocks'] is a flat list of inner blocks, but it would have to be further investigated if you plan to go this route.

Copy link
Contributor

Choose a reason for hiding this comment

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

Anyway, it largely depends on whether the Post Featured Image is the immediate child in the Post Template or it can be included at any level of nesting.

Oh.. that's true. It would make sense to have some metrics about this optimization whether it's better to recurse through innerBlocks to find if featured image exists in comparison to the two requests.

Copy link
Member Author

Choose a reason for hiding this comment

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

It would not be 2 requests, if you have 10 feature images on a page, it this could result in 10 times 2 ( one for post one for meta ) for each thumbnail. I have code in here to recursively loop through blocks to find nested navigation link blocks. I can confirm, that the inner is, is not a full list. The inner block, have inner blocks. So you have to recursively loop through blocks. It is a pain, but I have hidden the logic, so we could adapt it here.

The check here would be much simpler, as we just been a boolean of has feature image block.

@ntsekouras @gziolo Should I move the logic back to the post template block with a recursively lookup for featured-image blocks?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is also worthing noting, I dont have an issue with having this function in both the feature image and post template block. I don't think it hurts having it in both places.

Copy link
Contributor

Choose a reason for hiding this comment

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

@ntsekouras @gziolo Should I move the logic back to the post template block with a recursively lookup for featured-image blocks?

Yes, I think it's better to be there and pass the specific query. Also without some metrics I'm not even sure if it worths it to recurse through innerBlocks or just call the function 😄


$classnames = '';
if ( isset( $block->context['displayLayout'] ) && isset( $block->context['query'] ) ) {
Expand Down