pagination variable name should be unique

[tsarma]

Paginations variable name "page=" should be unique for news, search, events , etc ... For e.g. we have a site http://www.gewerbe-oftringen.ch/. On the right side there is a news module, which is restricted with max. of 3 entries. If there are more than 3 entires, then there will be a news pagination below. when a user seach the website from top right search box, this search result may also appear with pagination. Now suppose we have both pagination, and when the user clicks 2nd page from any of two paginations, there is probelm as both the link from news pagination and from search pagination attaches in url ?page=2. At the moment I've disabled pagination for the search result.

[Serhii-DV]

Similar problem here #3934

[tsarma]

Sadly on this page https://github.com/contao/core/issues/953 writes Leo, we are not supposed to have more than one pagination on same page. Closed!

[deerwood]

Multiple paginations are also a problem for my project (e.g. isotope product list and news list).

My idea/suggestion is, not to rename the page parameter, but instead change it's value from <page_no> to <page_no>:<module_id>. The module_id is the one requesting the new page. That way even multiple gallerys or news lists etc. would be possible. To avoid multiple page parameters and still keep the pagination state of other modules on the page, every triggered module saves its current page in the session.

The neccessary code change in the core would be moderate in my opinion:

• class Pagination had to accept a module id parameter in the constructor and generate the links accordingly. Also in the constructor the page parameter has to be checked and if the module ids don't match, the sessions current page for that module is taken instead ... or may be better, the current page is also a parameter to the constructor and the url parameter ignored
• the preparation code (8 places in 2.11.2) had to split and check the page parameters module_id, and use the page_no only, if it is meant, saves it in the session. Otherwise it takes the current page_no from the session. Finally instantiates a new `Pagination' object etc. as usual.

Third party extensions with pagination had to be adapted to fully support the new schema, but they wouldn't break completely, just react to pagination requests not meant for them, as it is now. The core on the other hand would not react wrongly to page parameters without a module_id.

I believe, this would be a cheap, but rather usefull enhancement to Contao.

[Toflar]

I think that's way too complicated! And it's not always the module id that is unique (e.g. several content elements using a pagination within the same article module). Why not just add a new parameter in the constructor for the $_GET key that is 'page' by default? It would be backward compatible for all the existing modules and provide the possibility of setting an own parameter the pagination is reacting to:

public function __construct($intRows, $intPerPage, $intNumberOfLinks=7, $strGetParam='page')

It would require some adjustments in the Pagination class itself because things like this

$this->strUrl = preg_replace('/\?.*$/', '', $this->Environment->request);

won't be possible anymore if you have more than one pagination active. But this is quite bad practice anyway as it kills the whole query string instead of just the page param :)

And then there are certainly a few checks in terms of the search indexing somewhere in the code but I think Leo knows best what needs to be done there :)

[deerwood]

Just a short note: I'm currently in progress of writing a proof of concept for my idea. That will be an extension, so that there is no need to patch the core directly. Probably I'll only support some of the 8 core preparation places (the ones I need in my project + one or the other isotope module + a paginatable version of tags_articles).

I believe, a better decision can be made with code/diffs at hand.

@Toflar: thanks for the hint to CEs. That is already solved easily in my proofs Pagination.php: in the generated pagination links the part after the colon with the id of module/CE now has a prefix m/c, e.g. page=2:c21 for the first gallery, page=2:c22 for the second gallery and page=2:m21 for the isotope list, all on the same page.

Besides, the original Pagination.php carefully rebuilds ALL url parameters (except page) directly after the mentioned preg_replace.

Does your concept mean, that there must be multiple $_GET params, one for each pagination present on a page? Without using the session?

[Toflar]

Right. What you are doing does not solve the problem. You want to paginate two modules, which means that you'd need something like this:

domain.tld/home.html?gallerypage=3&isotopepage=17

Or we make a service out of the pagination class so that it is a singleton. Then it could generate some kind of a special url parameter that combines the pages (here base64 encoded).

domain.tld/home.html?page=bTIyOjMsYzMzOjQ=

The session is no option as there would be no URI anymore.

[deerwood]

Toflar, thanks for pointing me / us into the right direction!

Multiple independent paginations in Contao are dead simple! So simple, that there is no need to show a diff here.

Essentially:

1. class Pagination:
   • accept a 4th param $strGetParam='page' as suggested by Toflar (and, sort of, by tsarma)
   • use that name instead of the hard coded 'page' in all places
2. preparation places:
   • just make a specific 'page' parameter, look for that, instead of the hard coded 'page' and supply it to the Pagination class

Code will follow soon. In the meantime see it in action:

• test.hamburgparadies.de/feuilleton.html
• test.hamburgparadies.de/test.html

Nice, isn't it?

[deerwood]

Here is the code, as an extension. Note, that all original files were taken from the official Contao 2.11.2 release and were only modified to make the multiple pagination work. Thus you must diff against that release, to see, what was changed by me. There are no other patches/fixes applied, so if you plan to use the extension until Contao (hopefully) has this feature in the core, be aware, that you have to apply any patches you did to the original files also to the files in the extension! And if 2.11.3 arrives you have to check and patch again eventually, it probably won't work with earlier releases etc.

I do not plan to release/support this extension officially, It's meant mainly as a proof of concept and it is my workaround for now, because in my current project (see above) multiple paginations are critical. Also the extension has to preload any classes involved in pagination and used in the project, which is totally ugly. So check, which paginations you really have togehter on one page and comment out all the others in config.php (Pagination.php must always preloaded).

Some comments: as told, the implementation is really easy and straight forward. If you look at the diff for Pagination.php you'll notice, that the <link rel="next/prev" ... feature (lately intruduced) is disabled. That is, because with 2 or more paginations on one page there is no clear next/prev and class Pagination, as it is, can't know, if the current page has only 1 pagination. It will be instantiatet for every pagination on a page and the instances don't know each other

As many sites might have no need for multiple paginations and the prev/next link is a good feature (acessibility and Google) I suggest, that a core implementation has a BE setting to enable multiple paginations, defaulting to off. When off, all will be as it is now: GET param is page everywhere; prev/next is enabled (there is no code in the proof for that, because you simply can disable the extension or not install it).

In the preparation places I use this schema for the GET parameter: a prefix ('mod' => 'tl_module', 'con' => 'tlcontent') and the id depending on the table used, followed by 'page', so that the GET parameters are always different for sure and one can have 2 different things of the same type on one page. See e.g. the test.html above, having 2 galleries. The comments module is a special case, because comments can refer to several different tables. In my implementation I just prefixed with 'com', then the given source table name and the parent id. That leads to rather long names evetually, e.g. com_tl_calendar_events42page und could be made better/shorter easily (my project will not have comments).

The typical idiom in the preparation places is:

@@ -109,7 +109,8 @@
             }

             // Get the current page
-            $page = $this->Input->get('page') ? $this->Input->get('page') : 1;
+            $page_param = 'mod' . $this->id . 'page';
+            $page = $this->Input->get($page_param) ? $this->Input->get($page_param) : 1;

             // Do not index or cache the page if the page number is outside the range
             if ($page < 1 || $page > ceil($total/$this->perPage))
@@ -134,7 +135,7 @@
             }

             // Add the pagination menu
-            $objPagination = new Pagination($total, $this->perPage);
+            $objPagination = new Pagination($total, $this->perPage, 7, $page_param);
             $this->Template->pagination = $objPagination->generate("\n  ");
         }

In a core implementation the first + line would check for the BE setting too. But otherwise, dead simple, isn't it?

Besides, there was no special change neccessary in Pagination.php to have multiple GET params building up when necessary (when several paginations are not on page 1). This puzzled me for a moment ... until I remembered, that the Pagination class carefully preserves the query string and just takes out the GET parameter at hand/given.

And a last comment on third party extensions with pagination, not yet conforming to the new feature: if you have only ONE such ext on any page, there is no problem. It will be the only one using a plane 'page' GET parameter. Look at the sample links in the previous message and in the left column; there is an Isotope product list, not yet patched ... no problem!

Conclusion: the proof shows, that the necessary changes are really cheap and straight forward. Multiple paginations per page are a usefull feature often. Though my current project might be an extreme case, there are many other combinations of pagionations possible. And because one could, as suggested, switch the feature on/off I really ask for a core change, hopefully in 2.11.3

Best regards, Georg

[deerwood]

I missed 2 places with hard coded 'page' parameters in 2.11.2 and fixed these (and a typo in my changes). The zip with my extension is updated accordingly. Also, for convenience, find here another zip with all diffs against public 2.11.2.

The patch in ´FrontendTemplate::output()` is really important and had a great impact on the search results ... much better!

Hopefully I spotted all involved places by now. Leo will know.

[leofeyer]

Changed in fcd72f5241b91ede2feb3cbc22cb6230371a759b.

[deerwood]

Hi Leo, thanks for accepting and implementing the essentials of multiple paginations.

However, in progress of inspecting fcd72f5 and testing, I have some issues with that commit:

1. Pagination.php: the change in line 231 (see https://github.com/contao/core/blob/fcd72f5241b91ede2feb3cbc22cb6230371a759b/system/modules/core/classes/Pagination.php#L231) to using strpos() was a bad idea, because it is a vague check. Typical case is the combination of a core and a third party extension, both paginating. The third party one will still use just page, and when processing that, page matches in fragments of, say page_n42 or mod42page etc. That leads to removal of parameters, that really should be preserved. The former strncasecmp() is much better (with adapted length) ... even consider to add the '=' in the check, to be really sure, that the fragment at hand is the one to be taken out / added later with different values.
2. Pagination.php: still happily generates rel="next/prev" LINK elements in HEAD unconditionally. In presence of multiple paginations that will generate MULTIPLE rel="next/prev" links for a page. This is not allowed / recommended / supported anywhere. Neither in Browsers, nor in specifications or by Google. Next/Prev links are only designed for ONE path through pages.
   • my initial suggestion was to have a BE setting to either allow multiple paginations without rel links, or to have rel links and forbid multiple paginations by means of forcing the pagination parameter to always be page. But I just discovered, that even a unique page parameter might genereate multiple "prev" links, when the max number of pages differ [EDIT: only with 3rd party extensions not doing what 2.11.2 does after "// Do not index or cache the page if the page number is outside the range "]. So that concept failed. One might call it a user error, to have multiple paginations on any page, when it is not allowed in the BE. But Contao should never genereate invalid output. That's why I disabled the rel links in my code.
   • I'm thinking about a new concept (brainstorming). Essentially along the line: pick only one of the several paginations to form a Google / user / browser "next/prev" sequence. The question naturally is, which one to pick, when there is a choice? And currently I have no good/clean idea how to implement it, except using some global state (e.g. $GLOBALS['MISC']['IS_PREV_NEXT_LINK_ALREADY_SET']).

Be patient with a follow up commit, there is even more to report ... it's just that I'm slow. In short (but I also have to check against 3.0):

• you did not notice my changes to FrontenTemplate.php, which are rather important (in 2.11.x)
• you changed Comments.php very differently from my version and just used $this->id, as in all other places. But I'm unsure if this is correct. Is there an ID at all? If so, does it give unique page params always? I'm in progress of testing.

Best regrads, Georg

[deerwood]

... continued ...

• ModuleListing.php: you missed another spot with a hardcoded page parameter (as I did at first, but corrected later) in line #197, 3.0 master:

    foreach (preg_split('/&(amp;)?/', $_SERVER['QUERY_STRING']) as $fragment)
    {
-       if ($fragment != '' && strncasecmp($fragment, 'order_by', 8) !== 0 && strncasecmp($fragment, 'sort', 4) !== 0 && strncasecmp($fragment, 'page', 4) !== 0)
+       if ($fragment != '' && strncasecmp($fragment, 'order_by', 8) !== 0 && strncasecmp($fragment, 'sort', 4) !== 0 && strncasecmp($fragment, $id, strlen($id)) !== 0)
        {
            $strUrl .= ((!$blnQuery && !$GLOBALS['TL_CONFIG']['disableAlias']) ? '?' : '&') . $fragment;
            $blnQuery = true;

still to be continued ...

[deerwood]

... continued (sorry for the delay) ...

• FrontendTemplate.php: here is the second place with a hardcoded page parameter you didn't change (and I missed in my first shot too). Look at the diff for 2.11.2 (in 3.0 it's around #L91 ff):

@@ -82,10 +82,16 @@
        }

        $strParams = '';
+       $arrPageParams = array();

        // Rebuild the URL to eliminate duplicate parameters
        foreach (array_keys($_GET) as $key)
        {
+           if (preg_match('/page$/', $key))
+           {
+               $arrPageParams[] = $key;
+               continue;
+           }
            if (!in_array($key, $arrIgnore))
            {
                if ($GLOBALS['TL_CONFIG']['useAutoItem'] && in_array($key, $GLOBALS['TL_AUTO_ITEM']))
@@ -101,10 +107,13 @@

        $strUrl = $this->generateFrontendUrl($objPage->row(), $strParams);

-       // Add the page number
-       if (isset($_GET['page']))
+       // Add the page numbers sorted (for the indexer)
+       sort($arrPageParams);
+       $blnQuery = false;
+       foreach ($arrPageParams as $page_param)
        {
-           $strUrl .= ($GLOBALS['TL_CONFIG']['disableAlias'] ? '&page=' : '?page=') . $this->Input->get('page');
+           $strUrl .= ((!$blnQuery && !$GLOBALS['TL_CONFIG']['disableAlias']) ? '?' : '&') . $page_param . '=' . $this->Input->get($page_param);
+           $blnQuery = true;
        }

        $this->keywords = '';

In the lower part of the patch (lines marked with "-") you see, that the original version just checked for page, the lines with "+" first collect all pagination params for later use and add them then, sorted, to the url.

The sort isn't essential, but usefull IMO. Several pagination params build up rather randomly, depending on the click path of the visitor. The sort normalizes the urls for the indexer.

Note, that the used/shown RegEx pattern /page$/ assumes a PREFIXED parameter name (e.g. x42_page instead of page_x42). But that can be changed easily ... just inform devs of third party extensions about your decision, so they can conform. I like the prefix much better.

• Comments.php: hmm, as far as I got, pagination for comments is only possible for content elements? If so, you are right with your version. @Toflar: please comment

I believe, I have found and pointed out ALL places, that have to be changed, to make multiple paginations finally possible and working well.

Now I would want to reopen this issue.

Best regards, Georg

[Toflar]

No, there's a content element and a module for the comments but both rely on Comments::addCommentsToTemplate()

[deerwood]

Finally I had the time to check Comments.php. Sorry to say, the commit fcd72f5 for Comments.php is plain wrong, because there is NO $this->id (at least in 2.11.3 and e.g. comments for news and I believe this is also true for 3.0).

Here my diff for Comments.php against 2.11.3 stable

@@ -65,7 +65,30 @@
            $total = $objTotal->count;

            // Get the current page
-           $page = $this->Input->get('page') ? $this->Input->get('page') : 1;
+           switch ($strSource)
+           {
+               case 'tl_content':
+                   $page_param = 'cc';
+                   break;
+               case 'tl_calendar_events':
+                   $page_param = 'ce';
+                   break;
+               case 'tl_faq':
+                   $page_param = 'cf';
+                   break;
+               case 'tl_news':
+                   $page_param = 'cn';
+                   break;
+               case 'tl_page':
+                   // ModuleComments, attention, intParent is page id
+                   $page_param = 'cp';
+                   break;
+               default:
+                   $page_param = 'c_' . $strSource;
+                   break;
+           }
+           $page_param .= $intParent . '_page';
+           $page = $this->Input->get($page_param) ? $this->Input->get($page_param) : 1;

            // Do not index or cache the page if the page number is outside the range
            if ($page < 1 || $page > max(ceil($total/$objConfig->perPage), 1))
@@ -86,7 +109,7 @@
            $offset = ($page - 1) * $objConfig->perPage;

            // Initialize the pagination menu
-           $objPagination = new Pagination($objTotal->count, $objConfig->perPage);
+           $objPagination = new Pagination($objTotal->count, $objConfig->perPage, 7 , $page_param);
            $objTemplate->pagination = $objPagination->generate("\n  ");
        }

Essentially the same as in my original version, but with shorter prefixes. The important thing is: $strSource must be used to build the prefix and instead of $this->id use $intParent.

Besides, I adapted my extension to 2.11.3. In case someone uses it / wants to see all diffs: both zips (see above) are updated accordingly.

[leofeyer]

All fixed in 858e3d30390d41d566d2f1499154bf32837eb358.

[deerwood]

Hi Leo,

thanks for the fix. I still have 2 issues with that last commit:

1. in Pagination.php #L212 you appended the = to the haystack of strpos() instead of the needle, where it belongs. Consider two fragments page_n42=3 (Core) and page=4 (third party extension). As it is, when processing the latter the needle is page and the haystacks are page_n42=3= and page=4= ... both fragments are taken out, but only page will be readded.
2. in Comments.php #L57 you calculate a 1 character $key with substr() from the tablename. For both tl_content and tl_calendar_events the key is 'c'. And for the [catalog] extension with tablenames like tl_catalog_items the key also would be 'c', leading to a pontential name clash of page_ccXX, when the $intParent XX is the same. The risk is low, but I wanted to point that out.

[leofeyer]

I am not very happy with no. 2, either, however I would neither be happy with a variable name like page_ctl_calendar_events256 in the URL. Maybe we should take the first character after each underscore? Then tl_calendar_events became page_cce256 and tl_catalog_items became page_cci256.

[deerwood]

Maybe we should take the first character after each underscore?

That is a good idea. Still not bullet proof, but the probability of name clashes would be almost zero.

[leofeyer]

Both fixed in 9eb9cc041f4d8626eb25661a212d96e3c34b171c.

[zonky2]

hmm... auf der Seite funktioniert das offensichtlich nicht

http://www.ametras.com/?page=3

hier werden News und Events gleichermaßen "geschaltet"