pageParameterName not supported

[verticka]

Bug Report

Q	A
BC Break	yes
Bundle version	6.6.1
Symfony version	7.1.5
PHP version	8.3.4

Summary

Parameter "pageParameterName" is not supported in links

Current behavior

Exemple with bootstrap 4: link is /suivi?page=2

How to reproduce

$paginator->paginate($repo->relanceDemande(),$request->query->getInt('pageRelanceDemande',1),50,['pageParameterName'=>'pageRelanceDemande']);

Expected behavior

link : /suivi?pageRelanceDemande=2

[garak]

I just tried and see that the expected behaviour is what I get.
Please provide more information.

[verticka]

your new knp_pagination_query function no longer supports "pageParameterName"

[garak]

The link is generated correctly for me, but the page is not advancing indeed.
Do you have a solution for that? Could you propose a fix?

[verticka]

Yes i have copied Older version
in my local template and change in configuration

[Jalliuz]

Same problem here:
When looking at this twig function
new TwigFunction('knp_pagination_query', [PaginationRuntime::class, 'getQueryParams']),
The getQeuryParams does not take the current paginator options into account and we always get the default 'page' instead of 'my-custom-page-name'

[garak]

Could you try this patch on your vendor knp-paginator-bundle dir?
After that, passing the pagination as the third argument of the knp_pagination_query Twig method should work.

--- a/src/Twig/Extension/PaginationRuntime.php
+++ b/src/Twig/Extension/PaginationRuntime.php
@@ -113,16 +113,18 @@ final class PaginationRuntime implements RuntimeExtensionInterface
      * @param int $page
      * @return array<string, mixed>
      */
-    public function getQueryParams(array $query, int $page): array
+    public function getQueryParams(array $query, int $page, ?SlidingPaginationInterface $pagination = null): array
     {
+        $pageName = $pagination?->getPaginatorOption('page_name') ?? $this->pageName;
+
         if ($page === 1 && $this->skipFirstPageLink) {
-            if (isset($query[$this->pageName])) {
-                unset($query[$this->pageName]);
+            if (isset($query[$pageName])) {
+                unset($query[$pageName]);
             }
 
             return $query;
         }
 
-        return array_merge($query, [$this->pageName => $page]);
+        return array_merge($query, [$pageName => $page]);
     }
 }

if you confirm it works, I'll release a patch version later today

[verticka]

Hello,
It doesn't work because I can't find the variable to put in your template model

It work for me :

`public function getQueryParams(array $query, int $page, ?string $pageParameterName = null): array
{
$pageName = $pageParameterName ?? $this->pageName;

    if ($page === 1 && $this->skipFirstPageLink) {
        if (isset($query[$pageName])) {
            unset($query[$pageName]);
        }

        return $query;
    }



    return array_merge($query, [$pageName => $page]);
}`

on Twig function add pageParameterName
knp_pagination_query(query, previous,pageParameterName)

[garak]

The variable is the pagination object passed to your template

[Jalliuz]

The solution of Vertica works indeed

public function getQueryParams(
    array $query,
    int $page,
    ?string $pageParameterName = null,
): array {
    $pageName = $pageParameterName ?? $this->pageName;

    if ($page === 1 && $this->skipFirstPageLink) {
        if (isset($query[$pageName])) {
            unset($query[$pageName]);
        }

        return $query;
    }
    return array_merge($query, [$pageName => $page]);
}

And in all places (all templates) change
knp_pagination_query(query, page) to knp_pagination_query(query, page, pageParameterName)

[garak]

I'm sure it works, but it would force use to extract the pageParameterName to be passed everywhere. Moreover, it's not future-proof: if one day we find in the need of another option, we would be forced to add a new argument.

[steveWinter]

@garak the issue with your proposed solution is that in the templates of the bundle we have no access to the SlidingPaginationInterface object.

For example, using the Bootstrap 5 template, getQueryParams is called via the twig function knp_pagination_query like this:

<li class="page-item">
    <a class="page-link" href="{{ path(route, knp_pagination_query(query, pageCount - 1)) }}">{{ pageCount - 1 }}</a>
</li>

And the parameters which are passed to the template are generated from the call to twig function knp_pagination_render which has parsed the SlidingPaginationInterface object into an array of parameters for the template - which as @Jalliuz points out includes pageParameterName.

I understand your concern about needing to pass pageParameterName 'everywhere' but if not that, then it has to be the SlidingPaginationInterface object which gets passed 'everywhere' and while this will make adding future requriements easier, it means changing the way the parameters in knp_pagination_render are computed.

If you're sure that's the way that you'd like to approach things, I can create a PR (because this bug is causing problems with an upstream library I'm trying to implement)?

[steveWinter]

PS I would also add, that using pageParameterName was how it was done in v5 - the same block form the template I shared above in v5 looked like this

<li class="page-item">
    <a class="page-link" rel="prev" href="{{ path(route, query|merge({(pageParameterName): previous})) }}">&laquo;&nbsp;{{ 'label_previous'|trans({}, 'KnpPaginatorBundle') }}</a>
 </li>

[garak]

I see your point.
Probably we should pass the entire set of options to the PaginationRuntime constructor, while currently we pass only a few.

Can you propose a PR?