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

Show all users in post author column. #106

Merged
merged 6 commits into from
Aug 18, 2022
Merged

Show all users in post author column. #106

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d680e22
Show all users in post author column.
Aug 15, 2022
e681ffd
Check users from the entire network in all cases
roborourke Aug 17, 2022
fd65bf3
Fix code style for comments
roborourke Aug 17, 2022
7fee064
Fix code style for comments
roborourke Aug 17, 2022
49bccb2
Add multisite specific tests
roborourke Aug 17, 2022
c271235
Fix code style issue, double quote -> single
roborourke Aug 17, 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
19 changes: 11 additions & 8 deletions inc/template.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,19 +21,21 @@
* @return int[] Array of user IDs.
*/
function get_author_ids( WP_Post $post ) : array {
if ( ! is_post_type_supported( $post->post_type ) ) {
if ( post_type_supports( $post->post_type, 'author' ) ) {
return [ intval( $post->post_author ) ];
$authors = wp_get_post_terms( $post->ID, TAXONOMY );
// If $authors is an error or empty.
if ( is_wp_error( $authors ) || empty( $authors ) ) {
// Check if the post_type supports Authorship.
if ( is_post_type_supported( $post->post_type ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This changes the existing logic but it’s not clear why. For any post type that doesn’t support authorship the function will now always return an empty array

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll test some more and see if I can tweak this further

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure why this change was needed at all to be honest, what lead you to altering it?

Copy link
Contributor Author

@robindevitt robindevitt Aug 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The function get_author_ids currently checks if a post type is not supported and then followed by doing a wp_get_post_terms which if the user is a super admin it returns an empty array.

So my thinking was to perform the check first for wp_get_post_terms if it returns an error or was empty then it would return the post author details. Looking at it now it could probably do with dropping the is_post_type_supported check too.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it can just be put back how it was - the problem was fixed just adding the blog_id => 0 argument right?

The change you have here will now cause a DB query even if the post type does not support multiple authors. Sometimes condensing logic like this doesn't help with performance or readability.

It's always good to do one thing at a time, ideally starting with conditionals that are the least effort to process, e.g. not checking the db.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

blog_id => 0 allows us to get the user who isn't assigned to the existing site when we have an ID. However with the current code in place for get_author_ids it returns an empty array still which will still show the reported issue.

So this change will ensure an author ID is returned.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could the problem be that when saving the author terms they aren't being stored if the user doesn't have a role on the current site?

This is still hiding the real issue I think - wp_get_post_terms( $post->ID, TAXONOMY ); should be returning IDs even for users that don't strictly have a role on the site.

// Check the post_type support author.
if ( post_type_supports( $post->post_type, 'author' ) ) {
return [ intval( $post->post_author ) ];
}
return [];
}

return [];
}

$authors = wp_get_post_terms( $post->ID, TAXONOMY );
if ( is_wp_error( $authors ) ) {
return [];
}

return array_map( function ( WP_Term $term ) : int {
return intval( $term->name );
}, $authors );
Expand All @@ -53,6 +55,7 @@ function get_authors( WP_Post $post ) : array {

/** @var WP_User[] */
$users = get_users( [
'blog_id' => 0,
roborourke marked this conversation as resolved.
Show resolved Hide resolved
'include' => $author_ids,
'orderby' => 'include',
] );
Expand Down