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

Jump to bottom

Ensure that post thumbnail is cached in latest post block. #40571

Merged
merged 11 commits into from
May 10, 2022
Merged

Ensure that post thumbnail is cached in latest post block. #40571

merged 11 commits into from
May 10, 2022

Conversation

spacedmonkey
Copy link
Member

What?

Ensure that post thumbnails are correctly primed when using the last post block.

Why?

If this function is not called, it results in one query to post meta table. Calling update_post_thumbnail_cache primes all caches in one.

How?

Testing Instructions

Screenshots or screencast

@spacedmonkey spacedmonkey added [Type] Performance Related to performance efforts [Block] Latest Posts Affects the Latest Posts Block labels Apr 24, 2022
@spacedmonkey spacedmonkey self-assigned this Apr 24, 2022
@spacedmonkey spacedmonkey requested a review from ajitbohra as a code owner April 24, 2022 23:37
@spacedmonkey spacedmonkey changed the title Ensure tht post thumbnail is cached in latest post block. Ensure that post thumbnail is cached in latest post block. Apr 24, 2022
@spacedmonkey spacedmonkey mentioned this pull request Apr 24, 2022
Merged
@getdave
Copy link
Contributor

getdave commented Apr 26, 2022

@spacedmonkey I've reached out to a few folks to see if we can get a review from a PHP-focused dev on this.

@spacedmonkey
Safe gaurd.
92bb6a0 
Co-authored-by: Tonya Mork <[email protected]>
hellofromtonya
hellofromtonya approved these changes Apr 26, 2022
View reviewed changes
Copy link
Contributor

@hellofromtonya hellofromtonya left a comment

Choose a reason for hiding this comment

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

LGTM 👍

felixarntz
felixarntz reviewed Apr 26, 2022
View reviewed changes
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 Awesome work! Can you add a unit test, specifically around the cache bit for featured images? To ensure no additional request per featured image is fired.

@spacedmonkey
Copy link
Member Author

@felixarntz I would love to, but I don't where I would do this. That test should really live in core. Do you have any recommendation on how I would add a test here.

@felixarntz
Copy link
Member

@felixarntz I would love to, but I don't where I would do this. That test should really live in core. Do you have any recommendation on how I would add a test here.

I think it would still work within Gutenberg's codebase, no? I think if we don't add tests here, we probably won't later add them to core either, since Gutenberg largely just gets merged into core over time.

You could add a file class-block-library-latest-posts-test.php to https://github.com/WordPress/gutenberg/tree/trunk/phpunit and implement a test using parse_blocks there (see https://github.com/WordPress/gutenberg/blob/trunk/phpunit/class-block-library-navigation-link-test.php#L96 as reference example for how GB code already tests other specific blocks).

@hellofromtonya
Copy link
Contributor

I think it would still work within Gutenberg's codebase, no? I think if we don't add tests here, we probably won't later add them to core either, since Gutenberg largely just gets merged into core over time.

I agree with @felixarntz. The PHPUnit tests are also backported to Core along with the source code changes. But if there are no tests, then none are added to Core, meaning changes are backported without tests being added. Creating them now not only validates them during the R&D, stabilization, and maturity process in Gutenberg, but also ensures there are tests to backport once it lands in Core.

@spacedmonkey

This comment was marked as duplicate.

Sign in to view
@spacedmonkey

This comment was marked as duplicate.

Sign in to view
@spacedmonkey
Copy link
Member Author

@felixarntz @hellofromtonya Point taken, unit tests added. Worth noting, I don't like query counting tests, so I found another way of testing this.

@spacedmonkey spacedmonkey requested a review from gziolo May 3, 2022 13:47
@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 May 6, 2022
peterwilsoncc
peterwilsoncc reviewed May 9, 2022
View reviewed changes
Copy link
Contributor

@peterwilsoncc peterwilsoncc left a comment

Choose a reason for hiding this comment

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

A couple of notes in the tests.

It's a shame update_post_thumbnail_cache() requires a switch to WP_Query but I understand what's happening.

phpunit/blocks/render-last-posts-test.php Outdated Show resolved Hide resolved
phpunit/blocks/render-last-posts-test.php Outdated Show resolved Hide resolved
phpunit/blocks/render-last-posts-test.php Outdated Show resolved Hide resolved
phpunit/blocks/render-last-posts-test.php Outdated Show resolved Hide resolved
phpunit/blocks/render-last-posts-test.php Show resolved Hide resolved
anton-vlasenko
anton-vlasenko requested changes May 9, 2022
View reviewed changes
Copy link
Contributor

@anton-vlasenko anton-vlasenko left a comment

Choose a reason for hiding this comment

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

I have a few remarks about the test.
Thank you.

phpunit/blocks/render-last-posts-test.php Outdated Show resolved Hide resolved
phpunit/blocks/render-last-posts-test.php Outdated Show resolved Hide resolved
phpunit/blocks/render-last-posts-test.php Outdated Show resolved Hide resolved
phpunit/blocks/render-last-posts-test.php Outdated Show resolved Hide resolved
phpunit/blocks/render-last-posts-test.php Show resolved Hide resolved
anton-vlasenko
anton-vlasenko approved these changes May 10, 2022
View reviewed changes
Copy link
Contributor

@anton-vlasenko anton-vlasenko left a comment

Choose a reason for hiding this comment

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

LGTM.

@spacedmonkey spacedmonkey merged commit 9709027 into trunk May 10, 2022
@spacedmonkey spacedmonkey deleted the fix/prime-thumbnail-cache-in-recent-posts branch May 10, 2022 10:33
@github-actions github-actions bot added this to the Gutenberg 13.3 milestone May 10, 2022
@adamziel
Copy link
Contributor

I cherry-picked this to wp/6.0 branch to be included in WordPress 6.0 RC2 later today a2be55c

@adamziel adamziel removed 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 May 10, 2022
adamziel pushed a commit that referenced this pull request May 10, 2022
@spacedmonkey @peterwilsoncc
@anton-vlasenko @adamziel
Ensure that post thumbnail is cached in latest post block. (#40571)
a2be55c 
* Ensure tht post thumbnail is cached in latest post block.

* Safe gaurd.

Co-authored-by: Tonya Mork <[email protected]>

* Add unit tests.

* Improve test.

* Apply suggestions from code review

Co-authored-by: Peter Wilson <[email protected]>

* Feedback.

* Improve unit tests.

* Update phpunit/blocks/render-last-posts-test.php

Co-authored-by: Anton Vlasenko <[email protected]>

* Change variable name.

* Remove message.

Co-authored-by: Tonya Mork <[email protected]>
Co-authored-by: Peter Wilson <[email protected]>
Co-authored-by: Anton Vlasenko <[email protected]>
@gziolo
Copy link
Member

gziolo commented May 10, 2022

We will have to bring unit tests to WordPress core in a dedicated patch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@peterwilsoncc peterwilsoncc peterwilsoncc left review comments

@costdev costdev costdev approved these changes

@anton-vlasenko anton-vlasenko anton-vlasenko approved these changes

@Mamaduka Mamaduka Awaiting requested review from Mamaduka

@youknowriad youknowriad Awaiting requested review from youknowriad

@getdave getdave Awaiting requested review from getdave

@ajitbohra ajitbohra Awaiting requested review from ajitbohra ajitbohra is a code owner

@draganescu draganescu Awaiting requested review from draganescu

@adamsilverstein adamsilverstein Awaiting requested review from adamsilverstein

@adamziel adamziel Awaiting requested review from adamziel

@hellofromtonya hellofromtonya Awaiting requested review from hellofromtonya

@felixarntz felixarntz Awaiting requested review from felixarntz

@gziolo gziolo Awaiting requested review from gziolo

Assignees

@spacedmonkey spacedmonkey

Labels
[Block] Latest Posts Affects the Latest Posts Block [Type] Performance Related to performance efforts
Projects
No open projects
WordPress 6.0 Editor Tasks
Archived in project
Milestone
Gutenberg 13.3
Development

Successfully merging this pull request may close these issues.
9 participants
@spacedmonkey @getdave @felixarntz @hellofromtonya @adamziel @gziolo @peterwilsoncc @anton-vlasenko @costdev