PHP page pagination

Question

I think my code is sloppy can anyone give me advice to do it better?

This is a page pagination that get rows from mysql db and calculate the number of pages.

<div class="row">
    <div class="twelve columns tac">
        <?php

        //Check for the current page and select one of the two versions of "back" link.
        if (isset($_GET["page_num"]) && $_GET["page_num"] >= 2): ?>
            <a href="?page=blog&page_num=<?php echo($_GET["page_num"] - 1); ?>">Back</a>
        <?php else : ?>
            <span>Back</span>
        <?php endif;

        //looping through the number of pages and list page numbers.
        for ($i = 1; $i <= $totalPages; $i++): ?>
            <?php if ($i == $_GET["page_num"]) : ?>
                <span><?php echo $i; ?></span>
            <?php else : ?>
                <a href="?page=blog&page_num=<?php echo $i; ?>"> <?php echo $i; ?></a>
            <?php endif; ?>
        <?php endfor;

        //Check for the current page and select one of the two versions of "Next" link.
        if ($_GET["page_num"] < $totalPages): ?>
            <a href="?page=blog&page_num=<?php echo($_GET["page_num"] + 1); ?>">Next</a>
        <?php else : ?>
            <span>Next</span>
        <?php endif; ?>
    </div>
</div>

Answer 1

Checking if a variable is set

            if (isset($_GET["page_num"]) && $_GET["page_num"] >= 2): ?>

Here you check if $_GET["page_num"] is set before using it, which is correct. However, you don't do so in any of the later tests. Consider starting with something the code block with something like

$currentPage = isset($_GET['page_num']) ? $_GET['page_num'] : 1;

Now you can just use $currentPage instead. And it will always be set. It defaults to 1, as that is the normal pagination behavior (no selected page means the first page).

I also changed the double quotes to single quotes. In PHP, double quotes means that a string is open to variable interpolation. You aren't using variable interpolation, so you might as well use single quotes instead.

Later you can change the original line of code to

            if ($currentPage > 1) {
?>

Note that I also moved the closing ?> to its own line. This makes it much easier to tell when you switch from code to HTML.

I also switched from the non-standard : notation. Very little PHP code is written that way. The only time that I've actually seen it is in WordPress templates.

And I switched from >= 2 to > 1 because 2 is not a significant number here. What you are saying is that you don't want allow the back link on the first page, only on pages after the first. This translates to > 1 more directly.

Separating PHP and HTML

I'd tend to write this as

<?php
    $currentPage = isset($_GET['page_num']) ? $_GET['page_num'] : 1;
?>
    <div class="row">
      <div class="twelve columns tac">
<?php
    echo getPaginationLinks($currentPage, $totalPages);
?>
      </div>
    </div>

and define getPaginationLinks in a separate file with most of the original block. The getPaginationLinks function would build and return a string.

Note that I put the <?php> and ?> on separate lines and at the beginning of each line. This makes it easier to tell when you are switch from HTML to PHP code.

Don't switch context for no reason

You have

            for ($i = 1; $i <= $totalPages; $i++): ?>
                <?php if ($i == $_GET["page_num"]) : ?>

You could just as well write this

            for ($i = 1; $i <= $totalPages; $i++):
                if ($i == $_GET["page_num"]) :
?>

Then the compiler isn't switching from PHP context to HTML context just to print out some meaningless whitespace. As @tim said, in this case it would be even better to build the string in PHP code rather than mixing PHP and HTML. In other situations this construct would be more acceptable. But there's still no point in switching out of PHP context unless you want to display something before you switch back into it.

Answer 2

Yes, mixing PHP and HTML can get messy quite quickly.

If you do not want to switch to some templating engine, the easiest way to clean this up is to build the entire HTLM block in a PHP variable and then echo it:

$paginationHTML = '
<div class="row">
    <div class="twelve columns tac">';

if (isset($_GET["page_num"]) && $_GET["page_num"] >= 2) {
    $paginationHTML .= getPaginationLink($_GET["page_num"] - 1, "Back");
} else {
    $paginationHTML .= 'Back';
}

for ($i = 1; $i <= $totalPages; $i++) {
    if ($i == $_GET["page_num"]) {
        $paginationHTML .= $i;
    } else {
        $paginationHTML .= getPaginationLink($i, $i);
    }   
}

if ($_GET["page_num"] < $totalPages) {
    $paginationHTML .= getPaginationLink($_GET["page_num"] + 1, "Next");
} else {
    $paginationHTML .= 'Back';
}

$paginationHTML .= '
    </div>
</div>
';

echo $paginationHTML;

function getPaginationLink($number, $linkText) {
    return '<a href="?page=blog&page_num=' . htmlspecialchars($number, ENT_QUOTES, 'UTF-8') . '">' 
        . htmlspecialchars($linkText, ENT_QUOTES, 'UTF-8') . '</a>';
}

It is of course a matter of taste, but I find this a lot easier to read.

I also extracted some duplication to a function and removed the span tags which did not seem to have any purpose.

You could generalize the function even more, resulting in less code, but also possibly reduced readability:

$paginationHTML = '
<div class="row">
    <div class="twelve columns tac">';

$paginationHTML .= getPagination($_GET["page_num"] - 1, "Back", isset($_GET["page_num"]) && $_GET["page_num"] >= 2);

for ($i = 1; $i <= $totalPages; $i++) {
    $paginationHTML .= getPagination($i, $i, $i != $_GET["page_num"]);
}

$paginationHTML .= getPagination($_GET["page_num"] + 1, "Next", $_GET["page_num"] < $totalPages);

$paginationHTML .= '
    </div>
</div>
';

echo $paginationHTML;

// TODO comment
function getPagination($number, $linkText, $doLink) {
    if ($doLink) {
        return '<a href="?page=blog&page_num=' . htmlspecialchars($number, ENT_QUOTES, 'UTF-8') . '">' 
            . htmlspecialchars($linkText, ENT_QUOTES, 'UTF-8') . '</a>';
    } else {
        return htmlspecialchars($linkText, ENT_QUOTES, 'UTF-8');
    }
}

The XSS protection is not strictly necessary, but when writing a generic function which builds HTML, it's recommended, as the function may be reused in different contexts.

Answer 3

You should really consider using an orm like RedBean. In my own project, i got tired of writing hundreds of paginated queries. the methods were huge and complex and just a nightmare to manage. I replaced them all with two convenience functions:

// returns number of pages, current page, and results in an associative array.
public function paginateWithPageCount($sql,$params=[],$page=1, $per_page=20)
{
    //first we need to prepare our query to count the number of rows

    $sql2 = $sql; //copy sql at this point.
    // to calculate the total number of rows for the paginater, 
    // we need to delete the sql up to the `FROM` keyword
    $test = 'FROM';
    $n = strpos($sql2,$test);
    $keep = substr($sql2,$n+strlen($test));
    $sql2 = 'SELECT COUNT(id) AS num '.$keep;


    //we know that php is 0 based, but for presentation purposes pagination should be 1 based.
    //citing this knowledge, we always need to subtract `1` from $page;
    $page = (int) $page;
    if($page < 1){
        $page = 1;
    }
    $offset = abs($per_page * ($page - 1)); // page - 1 * per_page = offset

    //now we append our limit to the $sql var
    $sql.=' LIMIT '.$offset.','.$per_page;

    if(empty($params)){
        $data = \R::getAll($sql);
        $num = \R::getCell($sql2); //the getCell method returns the value of a single column of a single row query.
    }else{
        $data = \R::getAll($sql,$params);
        $num = \R::getCell($sql2,$params);
    }
    $num = $num > $per_page ? ceil($num/$per_page) : 1; //tada, now we have the total number of pages.

    return ['pages'=>$num,'current'=>$page,'results'=>$data];
}

//returns only results
public function paginate($sql,$params=[],$page=1, $per_page=20)
{
    //we know that php is 0 based, but for presentation purposes pagination should be 1 based.
    //citing this knowledge, we always need to subtract `1` from $page;
    $page = (int) $page;
    if($page < 1){
        $page = 1;
    }
    $offset = abs($per_page * ($page - 1)); // page - 1 * per_page = offset
    $sql.=' LIMIT '.$offset.','.$per_page;
    if(empty($params)){
        $data = \R::getAll($sql);
    }else{
        $data = \R::getAll($sql,$params);
    }
    return $data;
}