[PHP] core.pagination_generate_page_link

[dcz]

Hello,

An event to alter pagination links would be very handy in \phpbb\pagination\generate_page_link

This could be handled like in append_sid with something like :

Code: Select all

    /**
    * Generate a pagination link based on the url and the page information
    *
    * @param string $base_url is url prepended to all links generated within the function
    *                            If you use page numbers inside your controller route, base_url should contains a placeholder (%d)
    *                            for the page. Also be sure to specify the pagination path information into the start_name argument
    * @param string $on_page is the page for which we want to generate the link
    * @param string $start_name is the name of the parameter containing the first item of the given page (example: start=20)
    *                            If you use page numbers inside your controller route, start name should be the string
    *                            that should be removed for the first page (example: /page/%d)
    * @param int $per_page the number of items, posts, etc. to display per page, used to determine the number of pages to produce
    * @return string URL for the requested page
    */
    protected function generate_page_link($base_url, $on_page, $start_name, $per_page)
    {
        global $phpbb_dispatcher;
        $page_link_overwrite = false;

        /**
        * This event can override the generate_page_link() function
        *
        * To override this function, the event must set $page_link_overwrite to
        * the new URL value, which will be returned following the event
        *
        * @event core.pagination_generate_page_link
        * @param string $base_url
        * @param string $on_page
        * @param string $start_name
        * @param int $per_page
        * @var    bool|string    page_link_overwrite    Overwrite function (string URL) or not (false)
        * @since 3.1.0-RC3
        */
        $vars = array('base_url', 'on_page', 'start_name', 'per_page', 'page_link_overwrite');
        extract($phpbb_dispatcher->trigger_event('core.pagination_generate_page_link', compact($vars)));

        if ($page_link_overwrite)
        {
            return $page_link_overwrite;
        }

        if (!is_string($base_url))
        {
            if (is_array($base_url['routes']))
            {
                $route = ($on_page > 1) ? $base_url['routes'][1] : $base_url['routes'][0];
            }
            else
            {
                $route = $base_url['routes'];
            }
            $params = (isset($base_url['params'])) ? $base_url['params'] : array();
            $is_amp = (isset($base_url['is_amp'])) ? $base_url['is_amp'] : true;
            $session_id = (isset($base_url['session_id'])) ? $base_url['session_id'] : false;

            if ($on_page > 1 || !is_array($base_url['routes']))
            {
                $params[$start_name] = (int) $on_page;
            }

            return $this->helper->route($route, $params, $is_amp, $session_id);
        }
        else
        {
            $url_delim = (strpos($base_url, '?') === false) ? '?' : ((strpos($base_url, '?') === strlen($base_url) - 1) ? '' : '&');
            return ($on_page > 1) ? $base_url . $url_delim . $start_name . '=' . (($on_page - 1) * $per_page) : $base_url;
        }
    } 

Use case is when you want to alter pagination, mainly for url rewriting purposes, but could be handy in other cases too.

Providing the event with all param from generate_page_link is the simple option, we could imagine to use two event for route/base_url'd cases, but I don't think it would be that wise.

[rfdy]

We've had a similar issue (implemented slugs/SEO and additional filter arguments in the URL that need to be preserved). We also replaced the "start" URL argument with a "page" one, to be compatible with our old forum. This is how we did it:
Implement your own "pagination" service in your extension that extends phpbb's pagination. we called ours Service/SeoPagination. In the constructor you'll want to inject whatever extras you need (in our case an Service\Seo and the phpbb request). Then implement the generate_template_pagination() method, in it you have access to $base_url. Given the $base_url you can try to match it (regex or however else), then replace the $base_url with your own controller info, this makes use of a little known feature which is if the $base_url is an array then it'll pass the array to helper->route() to generate the url. You just need to make sure to define your routes so they can be used.

That should be it. In our case this fixed pagination for forums and threads making use of the slug for each as well as the page number and other filter arguments we might have (like category ID, etc..).

IMHO adding the event the pagination, although useful, might expose the event to too many listeners. Basically a single function for generating arbitrary URLs is a bad idea. It should be the domain of the Service via the Router to generate URLs, not some magic function that has no knowledge of the current request or context. (Or at the least pass a router into the pagination to generate it by just appending the page number, which is what the current pager does, though poorly documented).

I can post some code if you need a simple example.

[dcz]

Basically a single function for generating arbitrary URLs is a bad idea. It should be the domain of the Service via the Router to generate URLs, not some magic function that has no knowledge of the current request or context. (Or at the least pass a router into the pagination to generate it by just appending the page number, which is what the current pager does, though poorly documented).

What do you mean ? The event (and hook) in append_sid is a way worst example if you go that path, and the base_url can and should be passed to the event to let the ext dev do everything needed (like again in append_sid) with all the knowledge available (more than actually useful).


The new service proposal is interesting, I'm going to try that path to solve the zero edit challenge, it might even be faster to execute in the end.

++

[dcz]

Btw, your solution worked very smoothly.

Ty.

[rfdy]

glad it worked for you!

[Nicofuma]

What is the status of this request?

[dcz]

Well I don't know really.

To me extending the pagination service is more efficient, but it may not be as handy as an event for some extension developers. It could be more consistent to have an event (as there is one in append_sid, you kinda need one for pagination to end the "event-able" urls), but documenting that fact in the function code would as well be enough. Just a line of comment stating that no event is required here since you can extend.

It could also be argued that extending is identical to changing phpbb code since you are manipulating core code and you could mess a lot more than with events (especially when updates occurs).

So using an event seems safer but is certainly less efficient. I would choose efficiency, but this is my personal opinion and I would fully understand it if you where to add this event.

Sorry for not answering

[rfdy]

The big question is: should multiple extensions be able to hook into the same event? Since using the service override approach will only allow one extension to do so, does it make sense for multiple extensions to manipulate URLs, and does the order matter? Until those questions are answered, I don't think it would be wise to add the event.

[dcz]

Again, since there is one event in append_sid and ALL urls except pagination goes through it, it's a bit late for the questions you raise, even though they do make a lot of sense, especially the priority / order one.

Pagination is just the last non event-ed bit of url handling.

[rxu]

Ticket: https://tracker.phpbb.com/browse/PHPBB3-13059
PR: https://github.com/phpbb/phpbb/pull/2947