More term ordering fixes #406
Conversation
co-authors-plus.php
Outdated
| ) ); | ||
| wp_cache_set( $cache_key, $coauthor_terms, 'co-authors-plus' ); | ||
| } else { | ||
| $coauthor_terms = array(); |
There was a problem hiding this comment.
We should remove the else and just do the variable init + assignment before the if
There was a problem hiding this comment.
You're right, I considered that and got lazy.
co-authors-plus.php
Outdated
| * @param int $post_id ID of the post for which to retrieve authors. | ||
| * @return array Array of coauthor WP_Term objects | ||
| */ | ||
| function cap_get_coauthor_terms_for_post( $post_id = false ) { |
There was a problem hiding this comment.
Why do we have a default of false if we just return early in that case?
There was a problem hiding this comment.
I just like being explicit :) but I'll remove it.
co-authors-plus.php
Outdated
| 'orderby' => 'term_order', | ||
| 'order' => 'ASC', | ||
| ) ); | ||
| wp_cache_set( $cache_key, $coauthor_terms, 'co-authors-plus' ); |
There was a problem hiding this comment.
I think we need to catch (and not cache) when a WP_Error is returned by wp_get_object_terms as that could lead to some "interesting" bugs.
There was a problem hiding this comment.
Hmm, I guess we missed this in the original fix PR too. We'll get a WP_Error when the taxonomy doesn't exist, so I'll cache an empty array in this case. Otherwise, we could end up with lots of wp_get_object_terms() and the resultant unpleasant queries.
co-authors-plus.php
Outdated
| $coauthor_terms = array(); | ||
| } | ||
|
|
||
| return $coauthor_terms; |
There was a problem hiding this comment.
Thoughts on moving the logic for this function inside the coauthors_plus class (and this function can just call that)?
There was a problem hiding this comment.
Yeah, I did think about putting this into the class but thought it silly to force everyone to do global $coauthors just to make a simple function call. Your idea solves that though, so I'll do that!
|
Okay, all good. The failing test is still just that one from the earlier |
|
For the failed PHP7 tests: http://stackoverflow.com/a/42561590/7618202, https://core.trac.wordpress.org/ticket/39822 |
|
Failing test comments: #398 (comment) |
In Test_Author_Queried_Object, tests will fail when, by default, tables for subsite are created as temporary.
|
Thanks @trepmal ! Looks like those remaining issues are thanks to this core bug. Given that, are we safe to merge this @mjangda ? |
co-authors-plus.php
Outdated
| ) ); | ||
| // Cache an empty array if the taxonomy doesn't exist. | ||
| $coauthor_terms = ( is_wp_error( $cached ) ) ? array() : $cached; | ||
| wp_cache_set( $cache_key, $coauthor_terms, 'co-authors-plus' ); |
There was a problem hiding this comment.
I don't think we should be caching when there is an error.
There was a problem hiding this comment.
@philipjohn let me know if that changeset makes sense to you and we can merge.
There was a problem hiding this comment.
My worry about not caching errors is that a simple typo could easily result in wp_get_object_terms() triggering a lot more than we'd like, and the uncached queries that comes with it.
co-authors-plus.php
Outdated
| $coauthor_terms = wp_cache_get( $cache_key, 'co-authors-plus' ); | ||
|
|
||
| if ( false === $coauthor_terms ) { | ||
| $cached = wp_get_object_terms( $post_id, $this->coauthor_taxonomy, array( |
There was a problem hiding this comment.
Minor: $cached is not quite the best name here. We should probably swap the var names $coauthor_terms and $cached.
+ $cache_coauthors = wp_cache_get( $cache_key, 'co-authors-plus' );
+
+ if ( false === $coauthor_terms ) {
+ $coauthor_terms = wp_get_object_terms( $post_id, $this->coauthor_taxonomy, array(
...
It can lead to unexpected bugs and make it not clear why we cached an empty array (and therefore harder to debug).
No need to test PHP nightly for now; 7 + 7.1 are good for now. Drop WordPress 4.5.
|
Remaining test failures are because the version of Travis running for CAP doesn't have PHP5.2 and PHPCS errors. |
Following on from #390 we also needed to revert the change introduce in #367 which meant that the CLI commands would also be affected by the bug.
This change moves the fix from #391 into it's own helper function -
cap_get_coauthor_terms_for_post()to avoid unnecessary code duplication for the cache getting/setting. It updates the existing uses of the cache to use the helper, and fixes the CLI commands' use ofget_the_terms()to use our new helper instead.