Zend Framework

Zend_Paginator bug with caching

Details

  • Type: Bug Bug
  • Status: Resolved Resolved
  • Priority: Critical Critical
  • Resolution: Fixed
  • Affects Version/s: 1.6.0
  • Fix Version/s: 1.10.5
  • Component/s: Zend_Paginator
  • Labels:
    None

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

Issue Links

This issue duplicates:
ZF-6989 Zend_Paginator DbSelect should not use Select object in serialization Major Resolved

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Dolf Schimmel (Freeaqingme) added a comment -

Added formatting

Show
Dolf Schimmel (Freeaqingme) added a comment - Added formatting
Hide
Permalink
Chuck Reeves added a comment -

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.

Show
Chuck Reeves added a comment - 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.
Hide
Permalink
Chuck Reeves added a comment -

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;
	}
}
Show
Chuck Reeves added a comment - 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;
	}
}
Hide
Permalink
Marco Kaiser added a comment -

fixed with r22302

Show
Marco Kaiser added a comment - fixed with r22302

People

Vote (3)
Watch (5)

Dates

  • Created:
    Updated:
    Resolved:
Atlassian JIRA (v4.4.3#663-r165197) | Report a problem
Powered by a free Atlassian JIRA open source license for Zend Framework. Try JIRA - bug tracking software for your team.