[2438] Анализ кода CPagination

[miramir]

Опишу странности, которые заметил в этом классе, во время расширения его дял собственных нужд. Ошибки это или за этим скрыт глубокий смысл я не знаю.

1. Функция createPageUrl($controller,$page)

Код: Выделить всё

    public function createPageUrl($controller,$page)
    {
        $params=$this->params===null ? $_GET : $this->params; // !!!! обратите внимание, что если $this->params!==null GET массив не учитывается.
        if($page>0) // page 0 is the default
            $params[$this->pageVar]=$page+1;
        else
            unset($params[$this->pageVar]);
        return $controller->createUrl($this->route,$params);
    }
 

Смотрим описание указанной переменной

Код: Выделить всё

    /**
     * @var array the additional GET parameters (name=>value) that should be used when generating pagination URLs.
     * Defaults to null, meaning using the currently available GET parameters.
     * @since 1.0.9
     */
    public $params;
 

additional - насколько мне известно это "дополнительные", тогда почему не делается array_merge? Кроме того в коде того же CPagination есть строчка $_GET[$this->pageVar]=$value+1; в функции setCurrentPage, что в данном случае работать не будет. Подозреваю что это всё-таки баг, а не фича.

2. Дальше в том же классе есть функции:

Код: Выделить всё

    public function getOffset()
    {
        return $this->currentPage*$this->pageSize; //Здесь для перемножаются значения полученные через get методы с помощью магических методов __get
    }

    public function getLimit()
    {
        return $this->pageSize; //Здесь также используется геттер
    }
 

В данном случае обращение $this->currentPage в какой-то степени оправданно, так как геттер сложный, хотя я бы допусти вызвал getCurrentPage явно. А вот геттер для $this->pageSize состоит из простого return $this->_pageSize и ради этого вызывается достаточно сложный метод CComponent::__get, ну и простой getPageSize в придачу. Думаю правильнее здесь использовать явно $this->_pageSize

[samdark]

1.

Код: Выделить всё

$params=$this->params===null ? $_GET : $this->params; // !!!! обратите внимание, что если $this->params!==null GET массив не учитывается. 

Разве? По-моему наоборот:

Код: Выделить всё

if($this->params===null){
  $params=$_GET;
}
else {
  $params=$this->params;
}
 

Про additional не уверен. Нужны тесты.

2. Если вдруг метод станет сложнее, это уже не будет правильным и потянет за собой переписывание кода.

[miramir]

1. Собственно это я и сказал, что если параметры определены, то ссылка будет формироваться только с ними, а все параметры которые пришли через $_GET будут утеряны, что для списков с фильтрами передаваемыми через $_GET будет фатально. Дополнительные параметры были бы логичнее.
2. Тогда надовызывать явно методы getCurrentPage и getPageSize вместо магических методов. Это будет хоть на чуть чуть, но быстрее, так как избавит от поиска по открытым полям класса а потом по методам.

[samdark]

1. Да, да… это я невнимательно читаю. Этот пункт логичен.
2. Согласен.

[miramir]

Мне создавать баг в офф. трекере или вы сами всё решите?

[samdark]

Лучше создать и дать ссылку. Так я не забуду точно.

[miramir]

Вот создал баг http://code.google.com/p/yii/issues/detail?id=2438 , но я написал его на русском языке (с английским у меня плоховато), так что если не сложно переведи его на английский.

[miramir]

К стати я расширяю клас для возможности передачи через гет количества строк на странице, могу предоставить свой код. Не планируется ли ввода подобного функционала основную ветку? Иногда очень удобно то, что пользователь сам может выбрать сколько элементов на странице он хочет увидеть.

[samdark]

Да, интересная возможность, хотя реализуется и со стандартным классом очень легко.

[SpiLLeR]

А как такое можно реализовать со стандартным классом?
Свое решение подобное тоже есть)

[samdark]

Как-то вот так:

Код: Выделить всё

$criteria = new CDbCriteria();
if(!empty($_GET['order']))
  $criteria->order = $_GET['order'];
 
$count=Article::model()->count($criteria);
 
$pages=new CPagination($count);
$pages->pageSize=10;
$pages->applyLimit($criteria);
 
$models = Article::model()->findAll($criteria);
 

[miramir]

Скорее так:

Код: Выделить всё

$criteria = new CDbCriteria();
if(!empty($_GET['order']))
  $criteria->order = $_GET['order'];
 
$count=Article::model()->count($criteria);
 
$pages=new CPagination($count);
$pages->pageSize=isset($_GET['pagesize'])?$_GET['pagesize']:10;
$pages->applyLimit($criteria);
 
$models = Article::model()->findAll($criteria);
  

Но в этом коде нет проверки на максимальное значение $_GET['pagesize'], и вообще теоретически у $_GET['pagesize'] могут быть строго заданные дискретные значения. Получается что мы выносим кухню относящуюся к пагинации наружу, видимо в контроллер, что на мой взглядне очень верно.

[miramir]

Вот http://pastebin.com/zwcLUXYv к стати мой вариант расширения класса, правда большая часть кода это исправление ошибок указанных выше. Если объединить эти модификации с основным CPagination мне кажется может получиться очень приятный класс.

[miramir]

Немного исправленная версия моего класса http://pastebin.com/fW6VaqZ2. Текущий вариан мне кажется более юзабильным.

[samdark]

Фикснуто.