Add caching to comment feeds #2194
Conversation
boonebgorges
left a comment
boonebgorges
left a comment
There was a problem hiding this comment.
I realize that it's aesthetically disappointing not to use WP_Comment_Query here, but I think that the approach you've settled on is safer. Looks OK to me.
peterwilsoncc
left a comment
peterwilsoncc
left a comment
There was a problem hiding this comment.
I've left a few notes inline.
I'm a little concerned about the new method, otherwise a few suggestions for tests.
| $this->assertTrue( $q2->is_comment_feed() ); | ||
| $this->assertFalse( $q2->is_singular() ); |
There was a problem hiding this comment.
I'd split these in to separate tests, basically:
- test
is_*methods return expected values, counts are correct -- archive - test
is_*methods return expected values, counts are correct -- singular - test cache is hit
- test cache is invalidated -- archive, new comment
- test cache is invalidated -- singular new post without new comments (is posts cache last changed time has an affect)
There was a problem hiding this comment.
Not sure I understand the need to split these tests up? Can you explain why we would have that overhead?
There was a problem hiding this comment.
True, there will be a small amount of overhead but the use of shared fixtures in wpSetUpBeforeClass() will reduce most of that.
The reason I suggested splitting them up (or at least doing so differently) is that it will provide more information to developers if something is broken.
In this test, if $q2->is_comment_feed() === false then the test to ensure caching is running correctly won't run. If a code change has broken both, it would be helpful to know all at once.
There was a problem hiding this comment.
This is not about fixtures, it is about what we are testing. We are testing the cache, checking that is_comment_feed and is_singular are just ensuring that values I submitted to WP_Query forced those conditionals to be true. Checking all those values at once, is the value in test. Without the asserts apart, the test is not as useful.
|
Committed in https://core.trac.wordpress.org/changeset/53065 and finished up in https://core.trac.wordpress.org/changeset/53066 because I forgot to run |
1 similar comment
|
Committed in https://core.trac.wordpress.org/changeset/53065 and finished up in https://core.trac.wordpress.org/changeset/53066 because I forgot to run |
4 participants
Trac ticket: https://core.trac.wordpress.org/ticket/36904
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.