search

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

Radical change: high-volume + WPML order fix #18

Closed rmpel closed 2 years ago

rmpel commented 6 years ago

Hello Jake Goldman and cohorts :)

I'd like to submit to you a very radical change. If you are not interested, that's ok too, we'll be maintaining a fork ourselves :)

The change:

No longer use wp_update_post in batches to update the menu_order. While this works perfectly in low-volume sites without WPML, this breaks in high-volume sites with WPML (500+ posts with translations). It got so bad in the end that even a directly-in-database change of order 64 to 10, followed by an edit (without change) + save of the post in WP resulted in revert to order 64.

Now use a few database queries to do the same thing. this is about 30 times faster, and solves the problem of order-changes not being stored.

I realise this is a very drastic change that might break other things, but perhaps you can use it as inspiration at least. For us, this solves the problem, but we've only tested this in 1 website. I also realise that this patch does not result in nice code (old code kept as comment), but again, it's intended as inspiration :)

Thank you for your time!

helen commented 6 years ago

Hi @rmpel - thanks for opening this PR. Per the contributing guidelines, could you also open a separate issue describing the problem (using WP's update functions can be slow, especially when there are a lot of actions being fired)? That way we can discuss possible solutions to time out issues and how to handle missing hooks when using alternate methods, etc. in the issue to keep all the decision-making context in one place and then if we do decide to go the route of direct queries, we can do a code review here in the PR.

rmpel commented 6 years ago

see https://github.com/10up/simple-page-ordering/issues/19

jeffpaul commented 2 years ago

@dkotter tagging you here for review with the caveat that I'm considering dropping the Support Level here to Stable and such a major change like this might be more than we are likely to enact. Also noting this PR is several years old (sorry folks about that), so worth considering a couple different options before we actually get into deep code review here on any path towards merge/release. So, let me know what you think before proceeding too far on this PR.

dkotter commented 2 years ago

With how long this has been open, there will for sure need to be some refreshing of this PR. I think this addresses a legitimate potential issue but I don't think the approach taken here is one we would want to go with. I'd suggest we close this out but leave the issue open and we can always consider coming up with a proper solution for this.

jeffpaul commented 2 years ago

@rmpel apologies for the long drawn-out review here, but going to close this PR but as @dkotter noted we do want to consider a fresh, alternate approach to resolving this as part of the plugin. I appreciate your help in opening the PR and associated issue and hope we can find a way to get this to resolution and will ensure we credit you in any future merged change on the topic, thanks!