Issues

ZF-8731: Zend_Paginator bug with caching

Description

Hello,

I found a bug in Zend_Paginator. If we enable caching of queries to the database through Zend_Paginator:


$cache = Zend_Cache::factory('Core', 'File', $frontendOptions, $backendOptions);
Zend_Paginator::setCache($cache);

....and then run the page with the following code:


$news = new Default_Model_DbTable_News();
$select = $news->select();
$paginator = Zend_Paginator::factory($select);

$page = (int)$this->_getParam('page', 1);
$paginator->setItemCountPerPage(1);
$paginator->setCurrentPageNumber($page);
$this->view->result = $paginator;

...it will execute 4 queries:

0.00359 connect NULL 0.00019 SELECT COUNT(1) AS zend_paginator_row_count FROM news NULL 0.00067 connect NULL 0.0003 SELECT news.* FROM news LIMIT 1 NULL

Also, it always creates a new cache with different name even though it performed the same query. I found function in code, which is responsible for generating name of cache

return md5(serialize($this->getAdapter()) . $this->getItemCountPerPage());

The problem is that the serialized object (Zend_Paginator_Adapter_DbSelect) when Zend_Db_Profiler is enable, has also included a unique time measurement queries. So we can be 100% sure that the result of function md5() will be different every time. Also, when we execute the function serialize() of adapter, Zend_Db disconnect from the database.

I prepared an initial fix, which solves the problem with the cache and re-connecting to the database. Zend_Paginator_Adapter_DbSelect class should implement Serializable interface (although I think it should all implement adapters, not only DbSelect):


class Zend_Paginator_Adapter_DbSelect implements Zend_Paginator_Adapter_Interface, Serializable
{
...
    public function serialize()
    {
        return serialize($this->_select->__toString());
    }
    public function unserialize($data) {}
...
}

I hope that this bug will be fixed soon.

Regards deallas

Comments

Added formatting

Please note that:


public function serialize()
    {
        return serialize($this->_select->__toString);
    }

Should be


public function serialize()
    {
        return serialize($this->_select->__toString());
    }

__toString is a method call not a property.

There is still an issue with the above fix. The limit clause for the select statement will not be set until after the cache is checked. When the cache is checked initially the query might look like the following:

SELECT _table.* FROM _table ORDER BY id ASC

After the cache is checked a call to getItems() is made to the adapter which will add the limit clause making the select statement:

SELECT _table.* FROM _table ORDER BY id ASC LIMIT 10

That causes the generated md5 to change. The fix would be to save the cache key to a class member and set that key on the 1st call to Zend_Paginator::_getCacheId() and unset on each call to Zend_Paginator::setItemCountPerPage() and Zend_Paginator::setCurrentPageNumber()

Below is my suggestion to help fix this issue: Please note that _getCacheInternalId now accepts the page parameter


class Zend_Paginator implements Countable, IteratorAggregate
{
    //...snip
    /**
    * Saved the cache key to ensure that it will always be the same
    * from checking cache to saving.
    * @see http://framework.zend.com/issues/browse/ZF-8731
    * @var unknown_type
    */
    protected $_cacheKey = null;

    //...snip
    public function setCurrentPageNumber($pageNumber)
    {
        $this->_currentPageNumber = (integer) $pageNumber;
        $this->_currentItems      = null;
        $this->_currentItemCount  = null;
        $this->_cacheKey          = null;

        return $this;
    }

        //...snip
    public function setItemCountPerPage($itemCountPerPage)
    {
        $this->_itemCountPerPage = (integer) $itemCountPerPage;
        if ($this->_itemCountPerPage < 1) {
            $this->_itemCountPerPage = $this->getItemCountPerPage();
        }
        $this->_pageCount        = $this->_calculatePageCount();
        $this->_currentItems     = null;
        $this->_currentItemCount = null;
        $this->_cacheKey         = null;

        return $this;
    }
    
    //...snip
    /**
    * Makes an Id for the cache
    * Depends on the adapter object and the page number
    *
    * Used to store item in cache from that Paginator instance
    *  and that current page
    *
    * @param int $page
    * @return string
    */
    protected function _getCacheId($page = null)
    {
        if ($page === null) {
            $page = $this->getCurrentPageNumber();
        }
        return self::CACHE_TAG_PREFIX . $page . '_' . $this->_getCacheInternalId($page);
    }

    /**
    * Get the internal cache id
    * Depends on the adapter and the item count per page
    *
    * Used to tag that unique Paginator instance in cache
    *
    * @return string
    */
    protected function _getCacheInternalId($page = null)
    {
        if (null == $this->_cacheKey){
            if ($page === null) {
                $page = $this->getCurrentPageNumber();
            }

            $this->_cacheKey    = md5(serialize($this->getAdapter()) . $this->getItemCountPerPage() . $page); 
        }

        return $this->_cacheKey;
    }
}

fixed with r22302