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.

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

[Block Library - Query Pagination Numbers]: Fix first page's link #33629

Merged
merged 2 commits into from
Aug 9, 2021
Merged
Changes from all commits
Commits
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
22 changes: 22 additions & 0 deletions packages/block-library/src/query-pagination-numbers/index.php
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,28 @@ function render_block_core_query_pagination_numbers( $attributes, $content, $blo
'total' => $total,
'prev_next' => false,
);
if ( 1 !== $page ) {
/**
* `paginate_links` doesn't use the provided `format` when the page is `1`.
* This is great for the main query as it removes the extra query params
* making the URL shorter, but in the case of multiple custom queries is
* problematic. It results in returning an empty link which ends up with
* a link to the current page.
*
* A way to address this is to add a `fake` query arg with no value that
* is the same for all custom queries. This way the link is not empty and
* preserves all the other existent query args.
*
* @see https://developer.wordpress.org/reference/functions/paginate_links/
*
* The proper fix of this should be in core. Track Ticket:
* @see https://core.trac.wordpress.org/ticket/53868
*
* TODO: After two WP versions (starting from the WP version the core patch landed),
* we should remove this and call `paginate_links` with the proper new arg.
*/
$paginate_args['add_args'] = array( 'cst' => '' );
Copy link
Contributor

Choose a reason for hiding this comment

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

What does cst stand for here, is it just a random useless param?

Copy link
Contributor

@youknowriad youknowriad Jul 22, 2021

Choose a reason for hiding this comment

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

I feel ideally, we should add a dedicated argument/option in paginate_links to disable the default behavior of skipping format. That said, this is fine a stopgap solution.

Copy link
Member

Choose a reason for hiding this comment

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

So this causes ?cst to be added to every link? 🤔 Seems a bit unexpected/weird.

Why not just filter paginate_links_output?

Theoretically it could cause some confusion if someone's already using a query arg with this name and expects it to do something else.

Could you perhaps share what the expected output is with this change?

Agreed that this should be fixed with a patch in core.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What does cst stand for here, is it just a random useless param?

Yes.

Theoretically it could cause some confusion if someone's already using a query arg with this name

We can change this to whatever. That's why I didn't used a single character, as it would increase those chances. I'm open to any suggestions.

Why not just filter paginate_links_output?

This would require parsing the html string and I'm not sure it worths it and how complicated it can get with other filters having been applied before, to other parts of this html construction. An extra variable in core should be the best solution probably.

}
// We still need to preserve `paged` query param if exists, as is used
// for Queries that inherit from global context.
$paged = empty( $_GET['paged'] ) ? null : (int) $_GET['paged'];
Expand Down