Only Query for author ID if user is member of blog

[sboisvert]

In WP_Query if a user is part of the multisite install but is not part of the current blog the query is still altered from only finding public posts to finding public posts and those that are private and from that author_id. While this never has an impact on the returned end results, not having private and author ID as a parameter lets MySQL optimize the queries and in the circumstances this has been tested on (large site, lots of posts lots of users) makes them run faster. On a site without multisite installed this will have the same behavior.

[sboisvert]

Actually attaching the right file.

[boonebgorges]

Thanks for the patch, sboisvert. This isn't really a multisite-specific issue - single-site WP can have Subscribers too - but it does tend to be more salient in a large network. So a more appropriate check than is_multisite() && is_user_member_of_blog() might be current_user_can( 'edit_posts' ).

The problem with this suggestion is that users can change roles (or, on your original patch, can be added to/removed from a blog). So an Author who publishes private posts who is then demoted to a Subscriber (or removed from the blog, in multisite) will no longer be able to see the private posts that she previously wrote. An edge case, to be sure.

What do you think?

See also #30911.

[sboisvert]

Thanks for having a look Boone!

Regarding the edge case, while it might change the current behaviour, I'm not sure if current behaviour is expected behaviour (in regards to lowering someone's permissions and they can still see the private posts they authored. (But as you mention this is probably an edge case either way).

I agree that current_user_can() is more applicable in terms of what we want to achieve in terms of optimizing the queries and given that in some cases it can be a dramatic boost I would think it's worth it. Current_user_can would apply this optimization to many more people than the multisite and member of blog check and again even with the edge cases I feel like this is worth it.

I do agree with #30911 that a re-organization of the post status logic would be best.

Is there something I can help with for this ticket?

Thanks

[boonebgorges]

I'm not sure if current behaviour is expected behaviour

I agree with that, and I think that the proposed change is probably going to be "right" for a greater number of installs, but I wanted to draw attention to it.

Is there something I can help with for this ticket?

I guess I'd like two things:

1. A third (fourth, etc) opinion. @jeremyfelt, do you have any thoughts about the suggestions put forward here?
2. Benchmarks that demonstrate the performance improvements of dropping the post_author clause would make the case stronger too. sboivert, can you rig something up to get us some numbers?

[jeremyfelt]

The current_user_can() check makes sense. Having a single query would be a benefit for many authenticated subscriber page views. I'd agree that benchmarks would be nice to see to help quantify.

The edge cases I'm imagining are very edgy. I could see trying something funky with a custom post type, subscribers or a custom role, and post_author that also relies on the private flag, but then you're pretty far out there. I wouldn't be too concerned with the impact on default posts, site members, and roles.

[boonebgorges]

The edge cases I'm imagining are very edgy. I could see trying something funky with a custom post type, subscribers or a custom role, and post_author that also relies on the private flag, but then you're pretty far out there.

Can we talk through this a little bit? I do see some possible complications:

• We should not be literally checking for current_user_can( 'edit_posts' ) - the check needs to be post-type specific.
• A WP_Query can query for multiple post types, and it's possible that a user only have the edit_posts cap of some of those post types. In this case, I think we say: if a user has 'edit_posts' in at least one of the post types, add the post_status clause for each post type.

I'm having a hard time concocting a concrete case like the "far out" one that you sketched. But, just to be clear, in this case the worst that will happen is that certain users with changed roles will not see their legacy content, right?

[jeremyfelt]

Replying to boonebgorges:

I'm having a hard time concocting a concrete case like the "far out" one that you sketched. But, just to be clear, in this case the worst that will happen is that certain users with changed roles will not see their legacy content, right?

Poking some more, I'm not sure that edit_posts is low enough of a capability because of how post authors can be assigned when extending the core UI.

I started exploring with an example of private feedback from an instructor to students. The instructor writes the feedback (post), sets the student (post_author, role of subscriber), and sets the status as private so that an individual student has access to only their feedback.

Currently, the student would have access to the individual post and to those posts via an archive display. With the patch provided by @sboisvert, this would stay the same as the student is recognized as a member of the site. If we switch to a more restrictive cap check, the individual private post would be readable, but the archive display would no longer show the private posts.

Sticking with is_member_of_blog() seems to make more sense. Of course, the trade off is then running get_blogs_of_user() instead of the OR. A comparison of numbers would help here.

[boonebgorges]

Thanks, jeremyfelt. I think I see how 'edit_posts' could be too restrictive.

How about 'read'? All Subscribers should have this cap. On non-MS, it'll be functionally redundant with the is_user_logged_in() check a few lines above. On MS, it should limit to members of the blog, which is what sboivert initially wanted. The benefits over the suggested solution are (a) no ugly is_multisite() check, and (b) no pulling up blog details inside of is_user_member_of_blog().

[sboisvert]

Thanks jeremyfelt, boonebgorges,
I did notice that the call to is_user_member_of_blog() was "slow"(ish) and that's why I stuck the is_multisite() in front of it originally. I haven't looked in the code of the read permission but it does sound like an easy check to do that would be faster than is_user_member_of_blog (since that functions pulls all of the permissions on all of the sites that user is part of), the original intention of this was not to change behaviour but to allow for MySQL to better use the indexes by not having it pull both post statuses & author when we 'know' that those won't return true.
Now again with the read permissions I think we still have the edge case of someone was an author / contributor and got their permissions changes. But as mentioned above I don't think this is a feature.

If there's consesus that this is the best way forward I'll try to give a patch as well as some performance numbers this week.

[johnbillion]

is_user_member_of_blog() is much more performant since [33771].

[boonebgorges]

@sboisvert Do you still have an appetite to produce some benchmarks? We want to be sure that introducing is_user_member_of_blog() doesn't cause more harm than good.

[sboisvert]

Yup! I'll get these to you by June 3rd

[pbearne]

this code got moved to link-template.php

src/wp-includes/link-template.php:1913

<?php
                foreach ( $private_states as $state ) {
                        if ( current_user_can( $read_private_cap ) ) {
                                $where .= $wpdb->prepare( ' OR p.post_status = %s', $state );
                        } else {
                                $where .= $wpdb->prepare( ' OR (p.post_author = %d AND p.post_status = %s)', $user_id, $state );
                        }
                }

Can similar code changed so closing as won't fix