Fix some issues with global queries

[davidwebca]

Hi!

I was encountering a number of issues, so I thought I would open a pull request, if not to solve those issues right away, at least to pick your brain.

Multiple terms as query var

The first issue was that I couldn't paginate a post type archive page that uses a taxonomy to filter with multiple terms as query vars. Ex.: http://website.com/projects/?project-category[]=slug1&project-category[]=slug2. I ended up plainly removing the post_type addition and is_tax check at Pagi.php:71 and all that was in that block, since it was recreating wrongly the current global query. An argument could be made that is_tax shouldn't be true on that page since it's a custom post type archive, but the fact is that WordPress does differently: when terms are in query vars, it activates is_tax => true on WP_Query, hence the error. But, in the end, the is_tax block shouldn't even be needed because of all the query vars that we pluck a few lines up and should already be defined.

URLs with query strings getting re-encoded

Around LengthAwarePaginator.php:32, the functions that generate the current page, previous page and next page's URL are escaping by default and if there's a query string with multiple params, html characters get re-encoded. The only function down the line that allows us to not escape to avoid a double-escape situation is the get_pagenum_link function that I used to recreate get_next_posts_page_link and get_previous_posts_page_link.

Unecessary round trip to the database

I was wondering if there was an explanation for the (get_posts)[https://github.com/Log1x/pagi/blob/master/src/Pagi.php#L84] which makes an extra round trip to the database. In any case, I figured it was simply to create an Illuminate\Support\Collection to pass to LengthAwarePaginator and let the Laravel Paginators do their magic, but if there's no use to have the posts data in there, the better solution to optimize this was to simply create an empty collection with the range function and using the $query->found_posts to fill it up to the right amount which, in turns, allows the creation of the pagination correctly.

[Log1x]

Hey.

Really sorry about the delay. These are all great changes. I really appreciate it. Going to test it more and then I'll get it merged in.

[Log1x]

Unecessary round trip to the database

This was a very expensive query on larger sites – I'm dumb for doing this. Your solution is much better. 🙈

This reduced the query time for one of my larger sites categories from 2.69s seconds to 0.56s (without caching). 😍

[Log1x]

I'm curious what the is_single() conditional is for in nextPageUrl() and previousPageUrl()?

I'm not seeing much of a change without it.

[Log1x]

Going to merge this in but I'm definitely curious about the is_single() check.

[davidwebca]

Hi! No worries. I had to dig down to remember why. The is_single call comes from the fact that, previously, you were calling next_posts() and previous_posts() to generate the URL.
https://developer.wordpress.org/reference/functions/next_posts/

In the first few lines, it escapes the URL once and then get_next_posts_page_link calls get_pagenum_link which re-escapes... unless we pass "false" as a second parameter. And so I copied the code used by WordPress and they, in core, do a "is_single" check. I imagine it's only because this pagination cannot be used for the "single page" / "break page" type of pagination and only for global query pagination.

https://developer.wordpress.org/reference/functions/get_next_posts_page_link/

[Log1x]

Interesting! I suppose we leave it as-is then. 🎉

Thank you!