10up / simple-page-ordering

Order your pages and other hierarchical post types with simple drag and drop right from the standard page list.
https://wordpress.org/plugins/simple-page-ordering/
GNU General Public License v2.0
149 stars 23 forks source link

fix: increase posts_per_page to include potential excluded posts #49

Closed dinhtungdu closed 4 years ago

dinhtungdu commented 4 years ago

Description of the Change

In #38 which addressed VIP Go review, we removed post__not_in from the sibling query and used PHP to sort out the excluded posts. There is a problem with this approach, the sibling query may include "too much" excluded posts, which leads to the needed post doesn't go into the query. This PR increases the posts_per_page value to cover the excluded posts.

Update: I switched to use offset instead of increasing the posts_per_page value.

See the original report: https://wordpress.org/support/topic/simple-page-ordering-fails-to-save-new-order-spins-forever/

Alternate Designs

Benefits

Possible Drawbacks

Verification Process

Checklist:

Applicable Issues

Changelog Entry

dinhtungdu commented 4 years ago

@ryanwelcher the $excluded array can be very big (see below) for sites with thousands of posts. I refactored using offset for better performance. This PR is ready for review.

https://github.com/10up/simple-page-ordering/blob/6e6fd578d4f938b05cf639b7e3582bc6988e616c/simple-page-ordering.php#L275-L281

helen commented 4 years ago

Closing this in favor of #51 - while I understand how we got to this point, this PR makes the intention of the code basically indecipherable, and the original flag from the VIP bot in #37 is only a warning. If a human flags this again, we can discuss at that time, since performance issues with this are inherent, but for now, let's go with the original and more readable solution.