PHP Pagination class (Clase para paginar resultados)

Question

This is my first question in Code Review. I apologize in advance for using Spanish (my native language). I thought there was a spanish version of Code Review.

This class is meant for pagination. It produces the desired results and I forced errors to check and everything seems ok to me.

Here is the code: (Edited to suit CR language policy)

/*
 * Usage:
 *
 * $query = "select id, razon_social, nombre_comercial, esta_habilitado, fecha_alta from clientes";
 * $headers = array("ID", "R. Social", "N. Comercial", "Habilitado?", "Fecha Alta");

 * require("Pagination.php");
 * $pagination = new Pagination($query, 10, $_GET['page'] ?? 1, 1, $headers);
 * echo "<br>Results: " . $pagination->get_total_results() . " (pages: " . $pagination->get_total_pages().")";
 * echo "<br>" . $pagination->get_results();
 * echo "<br>" . $pagination->get_navigator();
 *
 */
class Pagination {

    /** Data source can be either a MySQL query or a multidimensional array
     * this variable is overwritten after validation so it holds the selected data
     * overwrite occurs around lines 78 and 102
     */
    private $data_source = null;
    /** amount of results */
    private $total_results = 0;
    /** total pages */
    private $total_pages = 0;
    /** results per page */
    private $results_per_page = 10;
    /** URL for navigator */
    private $url = null;
    /** HTML navigator */
    private $navigator = null;
    /** resulting HTML output */
    private $html_output = null;
    /** page number */
    private $page = 1;
    /** output format: 1:table (default), 2:multidimensional array */
    private $format = 1;
    /** column headers (depends on $this->format) */
    private $headers = array();

    /**
     *
     * @param string|array $data_source MySQL query o multidimensional array
     * @param number $results_per_page Number of results per page
     * @param number $page Page number to show
     * @param number $format 1: HTML table (default), 2: multidimensional array
     * @param array $headers If $format==1 array with column headers. If the amount doesn't equal field count it will be ignored
     * @throws Exception
     */
    function __construct($data_source=null, $results_per_page=10, $page=1, $format=1, $headers=array()) {
        global $mysqli;

        if (!filter_var($results_per_page, FILTER_VALIDATE_INT) or ($results_per_page < 1)) {
            throw new exception("Incorrect number of results per page");
        }
        $this->results_per_page = $results_per_page;

        if (!filter_var($page, FILTER_VALIDATE_INT) or ($page < 1)) {
            throw new Exception("Incorrect page number");
        }
        $this->page = $page;

        if (($format != 1) and ($format != 2)) $this->format = 1;
        if (!is_array($headers)) {
            throw new Exception("Invalid headers");
        }

        if (!isset($data_source) or
            (is_string($data_source) and (strlen($data_source) == 0)) or
            (is_array($data_source) and (count($data_source) < 1))) {
                throw new exception("Incorrect data source");
        }

        if (is_array($data_source)) { // not implemented yet
            if (!es_multidimensional($data_source)) {
                throw new exception("Data source isn't a multidimensional array");
            } else {
                $this->total_results = count($data_source);
                $this->data_source = $data_source;
                $this->total_pages = ceil($this->total_results / $results_per_page);
            }
        } else {
            try {
                $mysqli->query($data_source);
            } catch (Exception $e) {
                throw new Exception("Error on data source");
            }
            $this->total_results = $mysqli->affected_rows;
            if ($this->total_results < 1) throw new Exception("No results");
            $this->total_pages = ceil($this->total_results / $results_per_page);

            $offset = ($page - 1) * $this->results_per_page;
            if ($page == 1) $data_source .= " limit 0, " . $this->results_per_page;
            elseif (($page > 1) and ($page <= $this->total_pages)) $data_source .= " limit " . $offset . ", " . $this->results_per_page;
            else {
                throw new Exception("Unexpected error"); // don't figure out how to set this error message
            }
            try {
                $resultados = $mysqli->query($data_source);
            } catch (Exception $e) {
                throw new Exception("Error on data source");
            }
            $this->data_source = $resultados->fetch_all(MYSQLI_ASSOC);

            if (($this->format == 1) and isset($headers) and (count($headers) != $mysqli->field_count)) {
                $this->headers = array();
            } else {
                $this->headers = $headers;
            }
            unset($offset);
            unset($resultados);
        }

    }

    function get_total_results() {
        return $this->total_results;
    }

    function get_total_pages() {
        return $this->total_pages;
    }

    function get_results() {
        /* in a nearby future this method will be available for several formats using templates
         * for now this is more than enough
         */
        if ($this->format == 1) { // tabla

            $this->html_output = "\n".'<div class="table">';
            if (isset($this->headers)) {
                $this->html_output .= "\n" . '<div class="table-header">';

                foreach($this->headers as $header) {
                    $this->html_output .= "\n" . '<div class="table-cell-header">'.$header.'</div>';
                }
                $this->html_output .= "\n" . '</div>';
            }
            $this->html_output .= "\n" . '<div class="table-body">';
            foreach($this->data_source as $data) {
                $this->html_output .= "\n" . '<div class="table-row">';
                foreach($data as $value) {
                    $this->html_output .= "\n" . '<div class="table-row-cell">' . $value .'</div>';
                }
                $this->html_output .= "\n" . '</div>';
            }
            $this->html_output .= "\n" . '</div>';

            $this->html_output .= "\n" . '</div>' . "\n";
            return $this->html_output;

        }
        return $this->data_source; // array
    }

    function get_navigator() {
        // prepare url for navigator
        $this->url = ROOT_WEB . substr($_SERVER['PHP_SELF'], 1);
        if (strlen($_SERVER['QUERY_STRING']) == 0) { $this->url .= "?page="; }
        else {
            $this->url .= '?';
            foreach($_GET as $variable => $value) {
                if ($variable != "page") $this->url .= $variable . "=" . $value . "&";
            }
            $this->url .= "page=";
        }

        // prepare navigator
        if ($this->total_pages > 1) {
            $previous = null;
            $following = null;
            $this->navigator = "<div id=\"paginator\">%1";

            for ($count = 1; $count <= $this->total_pages; $count++) {
                if ($this->page != $count) $this->navigator .= "<a href='" . $this->url. "$count'>$count</a>";
                else {
                    $this->navigator .= "<a href='#'>$count</a>";
                    if ($this->page != 1) {
                        $previous = '<a href="' . $this->url.($count-1).'"><</a>';
                        $this->navigator = str_replace("%1", $previous, $this->navigator);
                    } else {
                        $this->navigator = str_replace("%1", "", $this->navigator);
                    }

                    if ($this->page < $this->total_pages) {
                        $following = '<a href="' . $this->url.($count+1).'">></a>';
                    }
                }
            }
            $this->navigator .= $following;
            $this->navigator .= "</div>";
        }
        return $this->navigator;
    }
}

Are there any suggestions on improving my code?

Answer 1

Yes. So many things must be improved here that I don't even know where to start.

The class structure

Probably the most important issue of all. Strictly speaking, it is not a class. It's rather a collection of different functions that were accidentally put into a class.

• a class should be doing exactly one thing. Yours does everything, from querying the database to outputting HTML.
• a class should be using properties on purpose, not just a fancy way to call a variable.
• a class should never ever use a global variable. Even for functions it's frowned upon, but for classes it's completely forbidden. A class should contain everything it needs, and never use some variable out of nowhere.
• a class should be universal. It shouldn't be rewritten every time it is used on another site or when design is changed.

Database pagination.

There are two major flaws with database pagination:

1. Your class doesn't support prepared statements, which means your SQL is prone to SQL injection.
2. You should never ever select all rows to count them. A separate query using SELECT count(*) FROM ... MUST be used instead.

Suggestions

It seems your class is not much about pagination but rather about outputting HTML tables. So you got to make it so. Running SQL queries looks absolutely alien here. You can always get an array from SQL query using some other code, and pass it as array already. It will make your code more specific and on purpose.

My advise would be to put classes away for a while, and concentrate on learning basic programming principles such as SQL, security, code structure, separation of concerns.

I would suggest to make just a set of functions each doing its distinct job:

• a function that runs SQL query and returns an array. But you must remember that such a function MUST support using parameters in SQL. Here is one I wrote and recommend.
• a function that accepts an array and outputs an html table from array
• a function that accepts the total number of rows, the page size, the current page number - and displays navigation

Some pseudocode:

// some input validation
$current_page = ...
$offset = ...

$num_records = query(...);
$page_records = query(... $offset, $page_size);
$html_table = html_table($page_records);
$navigation = html_pagination_navigation($num_records, $page_size, $current_page); 

After some learning, you may take a look at this class, that indeed does some pagination, in the sense of database pagination.

Other improvements

It's a very good thing that you are using exceptions. But you must use them properly

• use appropriate exception type. For example, when input argument is invalid, PHP has an exception exactly for this occasion: InvalidArgumentException
• never ever use exceptions to hide the actual error message. Your "Error on data source" is a really bad idea. Instead of informative and helpful error message from mysql, you have just useless text that that doesn't tell you how to fix the error.
• let PHP to do some input validation for you: add type hinting for function arguments. If you write a function definition like this function navigation(int $total_pages) and PHP will do that FILTER_VALIDATE_INT (or is_array() etc.) validation for you and throw appropriate exception.
• when table headers aren't provided, you can use array_keys($data_source[0])