KnpLabs / KnpPaginatorBundle

SEO friendly Symfony paginator to sort and paginate
http://knplabs.com/en/blog/knp-paginator-reborn
MIT License
1.76k stars 344 forks source link

pageParameterName not supported #815

Open verticka opened 1 month ago

verticka commented 1 month ago

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 commented 1 month ago

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

verticka commented 1 month ago

your new knp_pagination_query function no longer supports "pageParameterName"

garak commented 1 month ago

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 commented 1 month ago

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

Jalliuz commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

The variable is the pagination object passed to your template

Jalliuz commented 1 month ago

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 commented 1 month ago

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.