From 0abb4a5ad3a2d5de9215473f07693b5610ee8945 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Thu, 22 Aug 2013 14:29:06 +0200 Subject: [PATCH 1/6] [ticket/11805] Allow specifying the page in the route for pagination Instead of "app.php?controller=news&start=10" authors might prefer having the pagination in the route. So the sample link from above would be the same as "app.php?controller=news/page/2". With this patch that is easily possible. What you do is, you specify the $base_url and $start_name as follows: $base_url = $controller_helper->url('news/page/%d'); $start_name = '/page/%d'; // Part of the url that will be removed // for page 1 Now this will generate: "news" for start = 0 "news/page/1" will work aswell, but will not be created "news/page/2" for start = 10 PHPBB3-11805 --- phpBB/includes/functions.php | 51 +++++++++++++++++++++++++++++++----- 1 file changed, 45 insertions(+), 6 deletions(-) diff --git a/phpBB/includes/functions.php b/phpBB/includes/functions.php index e905375f4a..d786119671 100644 --- a/phpBB/includes/functions.php +++ b/phpBB/includes/functions.php @@ -2212,8 +2212,12 @@ function tracking_unserialize($string, $max_depth = 3) * * @param object $template the template object * @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 $block_var_name is the name assigned to the pagination data block within the template (example: ) * @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 $num_items the total number of items, posts, etc., used to determine the number of pages to produce * @param int $per_page the number of items, posts, etc. to display per page, used to determine the number of pages to produce * @param int $start_item the item which should be considered currently active, used to determine the page we're on @@ -2234,6 +2238,7 @@ function phpbb_generate_template_pagination($template, $base_url, $block_var_nam $on_page = floor($start_item / $per_page) + 1; $url_delim = (strpos($base_url, '?') === false) ? '?' : ((strpos($base_url, '?') === strlen($base_url) - 1) ? '' : '&'); + $page_in_route = strpos($start_name, '%d') !== false; if ($reverse_count) { @@ -2263,9 +2268,18 @@ function phpbb_generate_template_pagination($template, $base_url, $block_var_nam if ($on_page != 1) { + if ($page_in_route) + { + $page_url = ($on_page - 2 > 0) ? sprintf($base_url, (int) $on_page - 2) : str_replace($start_name, '', $base_url); + } + else + { + $page_url = $base_url . $url_delim . $start_name . '=' . (($on_page - 2) * $per_page); + } + $template->assign_block_vars($block_var_name, array( 'PAGE_NUMBER' => '', - 'PAGE_URL' => $base_url . $url_delim . $start_name . '=' . (($on_page - 2) * $per_page), + 'PAGE_URL' => $page_url, 'S_IS_CURRENT' => false, 'S_IS_PREV' => true, 'S_IS_NEXT' => false, @@ -2279,7 +2293,14 @@ function phpbb_generate_template_pagination($template, $base_url, $block_var_nam $at_page = 1; do { - $page_url = $base_url . (($at_page == 1) ? '' : $url_delim . $start_name . '=' . (($at_page - 1) * $per_page)); + if ($page_in_route) + { + $page_url = ($at_page != 1) ? sprintf($base_url, $at_page) : str_replace($start_name, '', $base_url); + } + else + { + $page_url = $base_url . (($at_page == 1) ? '' : $url_delim . $start_name . '=' . (($at_page - 1) * $per_page)); + } // We decide whether to display the ellipsis during the loop. The ellipsis is always // displayed as either the second or penultimate item in the list. So are we at either @@ -2317,9 +2338,18 @@ function phpbb_generate_template_pagination($template, $base_url, $block_var_nam if ($on_page != $total_pages) { + if ($page_in_route) + { + $page_url = sprintf($base_url, $on_page); + } + else + { + $page_url = $base_url . $url_delim . $start_name . '=' . ($on_page * $per_page); + } + $template->assign_block_vars($block_var_name, array( 'PAGE_NUMBER' => '', - 'PAGE_URL' => $base_url . $url_delim . $start_name . '=' . ($on_page * $per_page), + 'PAGE_URL' => $page_url, 'S_IS_CURRENT' => false, 'S_IS_PREV' => false, 'S_IS_NEXT' => true, @@ -2344,13 +2374,22 @@ function phpbb_generate_template_pagination($template, $base_url, $block_var_nam } $tpl_prefix = ($tpl_prefix == 'PAGINATION') ? '' : $tpl_prefix . '_'; - $previous_page = ($on_page != 1) ? $base_url . $url_delim . $start_name . '=' . (($on_page - 2) * $per_page) : ''; + if ($page_in_route) + { + $previous_page = ($on_page > 2) ? sprintf($base_url, $on_page - 2) : str_replace($start_name, '', $base_url); + $next_page = sprintf($base_url, $on_page); + } + else + { + $previous_page = $base_url . $url_delim . $start_name . '=' . (($on_page - 2) * $per_page); + $next_page = $base_url . $url_delim . $start_name . '=' . ($on_page * $per_page); + } $template_array = array( $tpl_prefix . 'BASE_URL' => $base_url, $tpl_prefix . 'PER_PAGE' => $per_page, - 'U_' . $tpl_prefix . 'PREVIOUS_PAGE' => $previous_page, - 'U_' . $tpl_prefix . 'NEXT_PAGE' => ($on_page != $total_pages) ? $base_url . $url_delim . $start_name . '=' . ($on_page * $per_page) : '', + 'U_' . $tpl_prefix . 'PREVIOUS_PAGE' => ($on_page != 1) ? $previous_page : '', + 'U_' . $tpl_prefix . 'NEXT_PAGE' => ($on_page != $total_pages) ? $next_page : '', $tpl_prefix . 'TOTAL_PAGES' => $total_pages, $tpl_prefix . 'CURRENT_PAGE' => $on_page, ); From f16a7e351f30e70692adb0f51605e4826b92d597 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Thu, 22 Aug 2013 14:36:22 +0200 Subject: [PATCH 2/6] [ticket/11805] Fix "jump to page" feature for pagination in routes PHPBB3-11805 --- phpBB/styles/prosilver/template/forum_fn.js | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/phpBB/styles/prosilver/template/forum_fn.js b/phpBB/styles/prosilver/template/forum_fn.js index 693211983f..800fadd972 100644 --- a/phpBB/styles/prosilver/template/forum_fn.js +++ b/phpBB/styles/prosilver/template/forum_fn.js @@ -40,10 +40,14 @@ function jumpto(item) { page = prompt(jump_page, on_page); if (page !== null && !isNaN(page) && page == Math.floor(page) && page > 0) { - if (base_url.indexOf('?') === -1) { - document.location.href = base_url + '?start=' + ((page - 1) * per_page); + if (base_url.indexOf('%d') === -1) { + if (base_url.indexOf('?') === -1) { + document.location.href = base_url + '?start=' + ((page - 1) * per_page); + } else { + document.location.href = base_url.replace(/&/g, '&') + '&start=' + ((page - 1) * per_page); + } } else { - document.location.href = base_url.replace(/&/g, '&') + '&start=' + ((page - 1) * per_page); + document.location.href = base_url.replace('%d', page); } } } From cf46487dea0c583d58fc7ad04178841731a92e18 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Thu, 22 Aug 2013 16:58:56 +0200 Subject: [PATCH 3/6] [ticket/11805] Do not generate the same link twice There is no need to generate the prev/next links twice PHPBB3-11805 --- phpBB/includes/functions.php | 30 ++++++++++-------------------- 1 file changed, 10 insertions(+), 20 deletions(-) diff --git a/phpBB/includes/functions.php b/phpBB/includes/functions.php index d786119671..11e8b38d25 100644 --- a/phpBB/includes/functions.php +++ b/phpBB/includes/functions.php @@ -2266,20 +2266,21 @@ function phpbb_generate_template_pagination($template, $base_url, $block_var_nam $end_page = ($total_pages > 5) ? max(min($total_pages, $on_page + 3), 5) : $total_pages; } + $u_previous_page = $u_next_page = ''; if ($on_page != 1) { if ($page_in_route) { - $page_url = ($on_page - 2 > 0) ? sprintf($base_url, (int) $on_page - 2) : str_replace($start_name, '', $base_url); + $u_previous_page = ($on_page > 2) ? sprintf($base_url, (int) $on_page - 1) : str_replace($start_name, '', $base_url); } else { - $page_url = $base_url . $url_delim . $start_name . '=' . (($on_page - 2) * $per_page); + $u_previous_page = ($on_page > 2) ? $base_url . $url_delim . $start_name . '=' . (($on_page - 2) * $per_page) : $base_url; } $template->assign_block_vars($block_var_name, array( 'PAGE_NUMBER' => '', - 'PAGE_URL' => $page_url, + 'PAGE_URL' => $u_previous_page, 'S_IS_CURRENT' => false, 'S_IS_PREV' => true, 'S_IS_NEXT' => false, @@ -2295,7 +2296,7 @@ function phpbb_generate_template_pagination($template, $base_url, $block_var_nam { if ($page_in_route) { - $page_url = ($at_page != 1) ? sprintf($base_url, $at_page) : str_replace($start_name, '', $base_url); + $page_url = ($at_page > 1) ? sprintf($base_url, $at_page) : str_replace($start_name, '', $base_url); } else { @@ -2340,16 +2341,16 @@ function phpbb_generate_template_pagination($template, $base_url, $block_var_nam { if ($page_in_route) { - $page_url = sprintf($base_url, $on_page); + $u_next_page = sprintf($base_url, $on_page + 1); } else { - $page_url = $base_url . $url_delim . $start_name . '=' . ($on_page * $per_page); + $u_next_page = $base_url . $url_delim . $start_name . '=' . ($on_page * $per_page); } $template->assign_block_vars($block_var_name, array( 'PAGE_NUMBER' => '', - 'PAGE_URL' => $page_url, + 'PAGE_URL' => $u_next_page, 'S_IS_CURRENT' => false, 'S_IS_PREV' => false, 'S_IS_NEXT' => true, @@ -2374,22 +2375,11 @@ function phpbb_generate_template_pagination($template, $base_url, $block_var_nam } $tpl_prefix = ($tpl_prefix == 'PAGINATION') ? '' : $tpl_prefix . '_'; - if ($page_in_route) - { - $previous_page = ($on_page > 2) ? sprintf($base_url, $on_page - 2) : str_replace($start_name, '', $base_url); - $next_page = sprintf($base_url, $on_page); - } - else - { - $previous_page = $base_url . $url_delim . $start_name . '=' . (($on_page - 2) * $per_page); - $next_page = $base_url . $url_delim . $start_name . '=' . ($on_page * $per_page); - } - $template_array = array( $tpl_prefix . 'BASE_URL' => $base_url, $tpl_prefix . 'PER_PAGE' => $per_page, - 'U_' . $tpl_prefix . 'PREVIOUS_PAGE' => ($on_page != 1) ? $previous_page : '', - 'U_' . $tpl_prefix . 'NEXT_PAGE' => ($on_page != $total_pages) ? $next_page : '', + 'U_' . $tpl_prefix . 'PREVIOUS_PAGE' => ($on_page != 1) ? $u_previous_page : '', + 'U_' . $tpl_prefix . 'NEXT_PAGE' => ($on_page != $total_pages) ? $u_next_page : '', $tpl_prefix . 'TOTAL_PAGES' => $total_pages, $tpl_prefix . 'CURRENT_PAGE' => $on_page, ); From d7119a50546859f8f488e0c46ff0964362ab877a Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Thu, 22 Aug 2013 17:00:35 +0200 Subject: [PATCH 4/6] [ticket/11805] Add unit tests for pagination PHPBB3-11805 --- tests/pagination/start_parameter_test.php | 111 +++++++++++++++++++++ tests/pagination/templates/pagination.html | 12 +++ 2 files changed, 123 insertions(+) create mode 100644 tests/pagination/start_parameter_test.php create mode 100644 tests/pagination/templates/pagination.html diff --git a/tests/pagination/start_parameter_test.php b/tests/pagination/start_parameter_test.php new file mode 100644 index 0000000000..785fe80aa4 --- /dev/null +++ b/tests/pagination/start_parameter_test.php @@ -0,0 +1,111 @@ +template, $base_url, 'pagination', $start_name, $num_items, $per_page, $start_item); + $this->template->set_filenames(array('test' => 'pagination.html')); + + $this->assertEquals(str_replace("\t", '', $expect), $this->display('test')); + } +} diff --git a/tests/pagination/templates/pagination.html b/tests/pagination/templates/pagination.html new file mode 100644 index 0000000000..7f2a329804 --- /dev/null +++ b/tests/pagination/templates/pagination.html @@ -0,0 +1,12 @@ +pagination + +:previous:{pagination.PAGE_NUMBER}:{pagination.PAGE_URL} +:current:{pagination.PAGE_NUMBER}:{pagination.PAGE_URL} +:ellipsis:{pagination.PAGE_NUMBER}:{pagination.PAGE_URL} +:next:{pagination.PAGE_NUMBER}:{pagination.PAGE_URL} +:else:{pagination.PAGE_NUMBER}:{pagination.PAGE_URL} + + +:u_prev:{U_PREVIOUS_PAGE} + +:u_next:{U_NEXT_PAGE} From 8c786e82a7346d778626056177a8210edec09dfe Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Fri, 23 Aug 2013 00:50:20 +0200 Subject: [PATCH 5/6] [ticket/11805] Rename test to match the function name PHPBB3-11805 --- .../{start_parameter_test.php => generate_template_test.php} | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) rename tests/pagination/{start_parameter_test.php => generate_template_test.php} (96%) diff --git a/tests/pagination/start_parameter_test.php b/tests/pagination/generate_template_test.php similarity index 96% rename from tests/pagination/start_parameter_test.php rename to tests/pagination/generate_template_test.php index 785fe80aa4..587a948583 100644 --- a/tests/pagination/start_parameter_test.php +++ b/tests/pagination/generate_template_test.php @@ -10,7 +10,7 @@ require_once dirname(__FILE__) . '/../../phpBB/includes/functions.php'; require_once dirname(__FILE__) . '/../template/template_test_case.php'; -class phpbb_pagination_start_parameter_test extends phpbb_template_template_test_case +class phpbb_pagination_generate_template_test extends phpbb_template_template_test_case { protected $test_path = 'tests/pagination'; From 6fd6324dd632ae2ddf31ed4bb79411d03ed8c8fb Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Mon, 16 Sep 2013 18:51:14 +0200 Subject: [PATCH 6/6] [ticket/11805] Move page-url generating code into a new function PHPBB3-11805 --- phpBB/includes/functions.php | 57 ++++++++++++++++++------------------ 1 file changed, 29 insertions(+), 28 deletions(-) diff --git a/phpBB/includes/functions.php b/phpBB/includes/functions.php index 11e8b38d25..91a80a897d 100644 --- a/phpBB/includes/functions.php +++ b/phpBB/includes/functions.php @@ -2205,6 +2205,32 @@ function tracking_unserialize($string, $max_depth = 3) } // Pagination functions +/** +* 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 URL for the requested page +*/ +function phpbb_generate_page_link($base_url, $on_page, $start_name, $per_page) +{ + + if (strpos($start_name, '%d') !== false) + { + return ($on_page > 1) ? sprintf($base_url, (int) $on_page) : str_replace($start_name, '', $base_url); + } + 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; + } +} /** * Generate template rendered pagination @@ -2237,8 +2263,6 @@ function phpbb_generate_template_pagination($template, $base_url, $block_var_nam } $on_page = floor($start_item / $per_page) + 1; - $url_delim = (strpos($base_url, '?') === false) ? '?' : ((strpos($base_url, '?') === strlen($base_url) - 1) ? '' : '&'); - $page_in_route = strpos($start_name, '%d') !== false; if ($reverse_count) { @@ -2269,14 +2293,7 @@ function phpbb_generate_template_pagination($template, $base_url, $block_var_nam $u_previous_page = $u_next_page = ''; if ($on_page != 1) { - if ($page_in_route) - { - $u_previous_page = ($on_page > 2) ? sprintf($base_url, (int) $on_page - 1) : str_replace($start_name, '', $base_url); - } - else - { - $u_previous_page = ($on_page > 2) ? $base_url . $url_delim . $start_name . '=' . (($on_page - 2) * $per_page) : $base_url; - } + $u_previous_page = phpbb_generate_page_link($base_url, $on_page - 1, $start_name, $per_page); $template->assign_block_vars($block_var_name, array( 'PAGE_NUMBER' => '', @@ -2294,22 +2311,13 @@ function phpbb_generate_template_pagination($template, $base_url, $block_var_nam $at_page = 1; do { - if ($page_in_route) - { - $page_url = ($at_page > 1) ? sprintf($base_url, $at_page) : str_replace($start_name, '', $base_url); - } - else - { - $page_url = $base_url . (($at_page == 1) ? '' : $url_delim . $start_name . '=' . (($at_page - 1) * $per_page)); - } - // We decide whether to display the ellipsis during the loop. The ellipsis is always // displayed as either the second or penultimate item in the list. So are we at either // of those points and of course do we even need to display it, i.e. is the list starting // on at least page 3 and ending three pages before the final item. $template->assign_block_vars($block_var_name, array( 'PAGE_NUMBER' => $at_page, - 'PAGE_URL' => $page_url, + 'PAGE_URL' => phpbb_generate_page_link($base_url, $at_page, $start_name, $per_page), 'S_IS_CURRENT' => (!$ignore_on_page && $at_page == $on_page), 'S_IS_NEXT' => false, 'S_IS_PREV' => false, @@ -2339,14 +2347,7 @@ function phpbb_generate_template_pagination($template, $base_url, $block_var_nam if ($on_page != $total_pages) { - if ($page_in_route) - { - $u_next_page = sprintf($base_url, $on_page + 1); - } - else - { - $u_next_page = $base_url . $url_delim . $start_name . '=' . ($on_page * $per_page); - } + $u_next_page = phpbb_generate_page_link($base_url, $on_page + 1, $start_name, $per_page); $template->assign_block_vars($block_var_name, array( 'PAGE_NUMBER' => '',