2
\$\begingroup\$

I wrote this Pagination Section. It shows links to pages 123456789 and so on on SERP (Keyword Search Result Pages). Like you see on Google SERPs. Even though they work, I'm not sure I finished them to the best or valid practices. I'm worried about the encoding functions (raw_urlencode, urlencode, intval, htmlspecialchar), whether I put them in the correct places or not. I don't want double encodings where when links get clicked then URLs show up like:

http%3A%2F%2Fzorg.com%2Fcat%2Fsubcat?var_1=value+1&var2=2&this_other=thing&number_is=13

instead of like:

http://nogdog.com/cat/subcat?var_1=value+1&var2=2&this_other=thing&number_is=12

The links (keyword search result pages 123456789 etc.) are in this format:

http://sitedomain.com/search.php?find=keyword&table=links&column=keywords&max=100&page=1

NOTE: The URL of the SERP will contain user's searched keywords. That means the SERP urls/links will contain user inputs as part of the url. The the url's query part's key is 'find' and the value of it is the user's searched or inputted keywords. Need to make sure user doesn't make malicious inputs as their keyword search that show-up as the SERP urls/links (links to Pages 123456789, etc.) on the pages. Hence, made use of the htmlspecialchars() function when outputting the page numbered links.

$_REQUEST['find']:keyword the user is searching.
$_REQUEST['$max']:max results per page.
$_REQUEST['$page']:page number.
$_REQUEST['$table']: Mysql Table.
$_REQUEST['$column']: MySql Table Column.

I haven't included the whole pagination script that queries the Mysql database. Just the Pagination Section that I need reviewed. (By Pagination Section, I mean the bottom part you see on the Searchengine Result Pages that display links to other search result pages (Pages: 23456789 etc.)).

I have written 2 sets of code. I need feedback regarding which one to stick to and which one to discard with reasons given. The lines where I need help, I have commented as follows:

//ARE htmlspecialchars() OK HERE ?
//IS intval() OK HERE ?
//Are htmlspecialchars() & intval() OK HERE ?
//ARE urlencode() & intval() OK HERE ?

Php Code 1

    //SECTION: PAGINATION SECTION TO NUMBER THE SERPS AND LINK THEM.
    $total_pages = ceil($row_count/$max);
    $i = '1'; 

    $selfpage = basename(__FILE__,''); //Echoes: url_encode_Template.php. Does not fetch the url's query terms (params & their values absent).
    $path = rawurlencode($selfpage); //IS rawurlencode() OK HERE ?
    $query_string_1 = '?find=' .urlencode($find) .'&table=' .urlencode($table) .'&column=' .urlencode($column) .'&max=' .intval($max); //ARE urlencode() & intval() OK HERE ?

    //WHICH WHILE LOOP IS BEST ?
    //WHILE LOOP NUMBER: 1
    while($i<=$total_pages)
    {
        $query_string_2 = '&page=' .intval($i); //IS intval() OK HERE ?
        //Full URL With $_GET params (Query Strings): https://localhost/search.php?search=cars&table=links&column=keyword&max=100&page=1
        $url = $path .htmlspecialchars($query_string_1) .htmlspecialchars($query_string_2); //ARE htmlspecialchars() OK HERE ? 

        if($page == $i)
        {
            //Bold the current Page numbered link.
            echo '<a href=' .'"' .$url .'"' .'>' .'<b>' .intval($i) .'</b>' .'</a>'; //IS intval() OK HERE ?
        }
        else
        {
            echo '<a href=' .'"' .$url .'"' .'>' .intval($i) .'</a>'; //IS intval() OK HERE ?
        }
        $i++;
    }

    echo '<br>';
    echo '<b>'; echo __LINE__; echo '</b>'; echo '<br>';

Php Code 2

    //SECTION: PAGINATION SECTION TO NUMBER THE SERPS AND LINK THEM.
    $total_pages = ceil($row_count/$max);
    $i = '1'; 

    $selfpage = basename(__FILE__,''); //Echoes: url_encode_Template.php. Does not fetch the url's query terms (params & their values absent).
    $path = rawurlencode($selfpage); //IS rawurlencode() OK HERE ?
    $query_string_1 = '?find=' .urlencode($find) .'&table=' .urlencode($table) .'&column=' .urlencode($column) .'&max=' .intval($max); //ARE urlencode() & intval() OK HERE ?

    //WHICH WHILE LOOP IS BEST ?
    //WHILE LOOP NUMBER: 2
    while($i<=$total_pages)
    {
        $query_string_2 = '&page=' .intval($i); //IS intval OK HERE ?
        
        if($page == $i)
        {
            //Bold the current Page numbered link.
            echo '<a href=' .'"' ."$path" .htmlspecialchars($query_string_1) .htmlspecialchars($query_string_2) .'"' .'>' .'<b>' .intval($i) .'</b>' .'</a>'; //Are htmlspecialchars() & intval() OK HERE ?
        }
        else
        {
            echo '<a href=' .'"' ."$path" .htmlspecialchars($query_string_1) .htmlspecialchars($query_string_2) .'"' .'>' .intval($i) .'</a>'; //Are htmlspecialchars() & intval() OK HERE ?
        }
        $i++;
    }
}
\$\endgroup\$
0
3
\$\begingroup\$

Your questions are very detailed, I might even say, a bit too detailed for reviewing. The code you have is not very complex, but it contains quite a few things I noticed:

  • Why is $i = '1'; a string, when all it ever contains is a number?
  • Some of the variable names you choose are not very meaningful. They don't reflect the meaning of the content of the variable. I mean $i and $max.
  • The difference between the two versions is minimal, it would have been better to just present one.
  • As already mentioned by mickmackusa, you could use http_build_query() here.
  • I like the use of basename(__FILE__) since $_SERVER['PHP_SELF'] is not always completely reliable. However, the PHP script must be in the root of the website. A good alternative is $_SERVER['SCRIPT_NAME'].

Let me try a rewrite:

$total_pages = ceil($row_count / $max);
$page_no     = 1; 
$url         = $_SERVER['SCRIPT_NAME'] . '?' .
               http_build_query(['find'   => $find,
                                 'table'  => $table,
                                 'column' => $column,
                                 'max'    => $max]); 

while ($page_no <= $total_pages)
{
    $page_str = ($page == $page_no) ? "<b>{$page_no}</b>" : $page_no;
    echo "<a href=\"{$url}&page={$page_no}\">{$page_str}</a> ";
    $page_no++;
}

Notice how all the intval() problems have disappeared? I use a single string, after the echo for output. The \" backslash in front of the double quotes means that these have to be taken literal, and don't signify the end of the string. I surrounded the variables with {$var}, to make them stand out a bit more, but in practice this is not needed. I also added an extra space after the </a> to make the numbers, which can be clicked, move a bit futher apart.

I hope you have limited the amount of results, because all pages will be represented.

(code is untested, I do not exclude unforeseen problems, sorry)

mickmackusa suggested to use for() and printf() in a comment. Below you see my idea of what he could have meant by that.

$total_pages = ceil($row_count / $max);
$url         = $_SERVER['SCRIPT_NAME'] . '?' .
               http_build_query(['find'   => $find,
                                 'table'  => $table,
                                 'column' => $column,
                                 'max'    => $max]); 

for ($page_no = 1; $page_no <= $total_pages; $page_no++)
{
    $page_str = ($page == $page_no) ? "<b>$page_no</b>" : $page_no;
    printf('<a href="%s&page=%d">%s</a> ', $url, $page_no, $page_str);
}
\$\endgroup\$
9
  • \$\begingroup\$ How about leveraging for() and printf()? \$\endgroup\$ Sep 23 '21 at 12:23
  • 2
    \$\begingroup\$ @mickmackusa Those are good ideas, why not add an answer with that? An answer doesn't have to be a complete review. Feel free to copy my code and improve it. \$\endgroup\$ Sep 23 '21 at 13:23
  • \$\begingroup\$ @mickmackusa, If you improve KIKO Software code then kindly paste the improvement in this thread for everyone's benefit. - Thanks! \$\endgroup\$ Sep 23 '21 at 17:27
  • \$\begingroup\$ @KIKO Software, I already done a pagination section with http_build_query() recently. However, your version is more shorter in code. I'd love to memorise your code. The 2 php codes I mentioned in this thread were made before I learnt about http_build_query(). I still prefer to learn how to finish this project without the http_build_query() and so if it's not too much to ask nor a bother to you then do you mind updating atleast one of my 2 codes mentioned in my op (without using http_build_query()? I'm a beginner programmer and still at procedural style programming. \$\endgroup\$ Sep 23 '21 at 17:31
  • 1
    \$\begingroup\$ I am trying to save my marriage by spending far less time contributing in the Stack Exchange universe. My SE obsession is putting strain on my real life. \$\endgroup\$ Sep 23 '21 at 21:01
1
\$\begingroup\$

Avoid intval if you can

        $query_string_2 = '&page=' .intval($i); //IS intval() OK HERE ?

It is OK to use intval there but unnecessary for two reasons. First

        $query_string_2 = '&page=' .(int)$i;

It is more efficient to cast to int than to use intval. Because intval is a function. Whereas casting is built into the PHP interpreter/compiler. The times when you would want to use intval are when you need a function. E.g. with array_map. But as others have noted, it is not necessary to cast to int at all if you are generating it yourself. You already know that $i is valid, because you created it from a constant value and only increment it. The only reason to cast it is to make it easier to see that it is an integer.

basename(__FILE__) caveats

$selfpage = basename(__FILE__,''); //Echoes: url_encode_Template.php. Does not fetch the url's query terms (params & their values absent).

This is OK, but it will only work if your page is in the root directory of the base URL. That will be the root directory of the site if you don't set it to something else explicitly. It may also fail if you are using something like mod_rewrite, but there are a lot of things that can go wrong that way. The mod_rewrite extension is powerful and expressive, which makes it easy to use it to break things.

$selfpage = basename(__FILE__);

You don't need to set the second parameter. The default will do what you want.

URL encoding a path

$path = rawurlencode($selfpage); //IS rawurlencode() OK HERE ?

That has to be rawurlencode rather than urlencode because encoding spaces as + doesn't work in the path. A lot of the time, you won't have to do anything here. Because most file names are the same whether URL encoded or not. But if you want to allow arbitrary file names here, then rawurlencode is the way to go.

But note that rawurlencode will encode /. So you have to be sure that your value won't have any of those. In this case, you use basename for that. Another alternative is to explode, array_map, and implode. E.g.

$path = implode('/', array_map(explode('/', $untrusted), 'rawurlencode'));

Note that you can use either urlencode or rawurlencode there. The rawurlencode function is more reliable, while the urlencode function is more human readable.

You'd only want to use that in a path. It makes no sense in a query parameter, where you do want to encode /.

& is a special character in HTML

            echo '<a href=' .'"' ."$path" .htmlspecialchars($query_string_1) .htmlspecialchars($query_string_2) .'"' .'>' .'<b>' .intval($i) .'</b>' .'</a>'; //Are htmlspecialchars() & intval() OK HERE ?

The only reason that I can see to use htmlspecialchars here is to convert & to &amp;. But since you want none of the rest of the behavior, you would be better off with str_replace('&', '&amp;', $string) to do that.

\$\endgroup\$
3
  • \$\begingroup\$ So, when I get page number from url via $_GET['page_no'], I should cast it int(page_no) over the intval($page_no). Yes ? \$\endgroup\$ Sep 24 '21 at 16:39
  • \$\begingroup\$ To make sure the query 'page_no=' is containing an INT and no chars or symbols. Gonna cast like so: int($page_no)=$_GET['page_no']; That's ok, right ? \$\endgroup\$ Sep 24 '21 at 16:48
  • 1
    \$\begingroup\$ (int) is an operator, like unary plus. It is not a function call. I'm not sure that int($page_no) will work. It might need to be ((int)$page_no) if you need the extra parentheses for some reason. \$\endgroup\$
    – mdfst13
    Sep 24 '21 at 18:50
0
\$\begingroup\$

I updated KIKO Software's code as he forgot to add the "?" at the end of:

$url = $_SERVER['SCRIPT_NAME'];

And added these:

$search = $_GET['search'];
$table = $_GET['table'];
$column = $_GET['column'];
$max = $_GET['max'];
$page_no = $_GET['page_no'];

$row_count = 10; //Typical Example. (This depends on row matches on the Mysql Query).

UPDATE

<?php

$find = $_GET['find'];
$table = $_GET['table'];
$column = $_GET['column'];
$max = $_GET['max'];
$page_no = $_GET['page_no'];

$row_count = 10;
$total_pages = ceil($row_count / $max);

$url = $_SERVER['SCRIPT_NAME'].'?'. 
http_build_query(['find'   => $find,
                  'table'  => $table,
                  'column' => $column,
                  'max'    => $max]); 

$page = 1;
while ($page <= $total_pages)
{
    $page_str = ($page == $page_no) ? "<b>{$page}</b>" : $page;
    echo "<a href=\"{$url}&page_no={$page}\">{$page_str}</a> ";
    $page++;
}

?>
\$\endgroup\$
6
  • 1
    \$\begingroup\$ Ah, yes, I forgot the question mark, I added it. Sorry, I saw no reason to try to write code without the perfectly adequate http_build_query(). \$\endgroup\$ Sep 23 '21 at 19:46
  • \$\begingroup\$ @KIKO Software, writing code without the perfectly adequate http_build_query() would teach me where my pitfalls are when using the encoders. htmlspecialchars(), htmlentities(), intval, (val), urlencode(), raw_urlencode(), etc. the whole point of me opening this thread. When using http_build_query, I understand now, none of these functions usage are really necessary. Let's say, I find myself in a fix where I need to use those functions and http_build_query() is irrelevant there. Then, I'd use those functions incorrectly. Nevermind, your choice. \$\endgroup\$ Sep 24 '21 at 15:06
  • \$\begingroup\$ @KIKO Software, Reason why I said I already got the http_build_query() version is because I already got help on the following link before I opened this particular thread. However, your shorter version here is better than the one is StackOverFlow, I reckon and so thanks for it no matter what. stackoverflow.com/questions/69238628/… \$\endgroup\$ Sep 24 '21 at 15:07
  • \$\begingroup\$ @KIKO Software, I prefer to use echo over printf. printf just more to write in terms of params. And prefer to use WHILE loop instead of the FOR LOOP as latter got more to write. \$\endgroup\$ Sep 24 '21 at 15:08
  • \$\begingroup\$ @mdfst13, Sorry, but can you explain more about the $untrusted as I didn't quite understand that part. Do use my own url as an example. - Thanks! \$\endgroup\$ Oct 4 '21 at 16:59

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.