2.8 _getCacheInternalId() is too generic

[richard-parnaby-king]

In Zend\Paginator\Paginator::_getCacheInternalId() (lines 863 - 871) the md5 hash of the adaptor being generated matches all adaptor instances. The issue is on line 868:

. json_encode($this->getAdapter())

This sucker is returning '{}' for all my table adaptors (adaptor being supplied is Zend\Paginator\Adapter\DbSelect).

I don't understand the reason for replacing spl_object_hash, but I do ask how do I get my paginator instances working again?

[froschdesign]

@richard-parnaby-king

This sucker is returning '{}' for all my table adapters…

I can not reproduce this problem. I get always a correct / usable cache ID.

Please test this short script:

$select    = new Zend\Db\Sql\Select('…'); // add a table name
$dbAdapter = new Zend\Db\Adapter\Adapter(
    [
        'driver'   => '…', // add your database config
        'database' => '…',
        'username' => '…',
        'password' => '…',
    ]
);
$adapter   = new Zend\Paginator\Adapter\DbSelect($select, $dbAdapter);
$paginator = new Zend\Paginator\Paginator($adapter);

$cache = Zend\Cache\StorageFactory::adapterFactory(
    Zend\Cache\Storage\Adapter\BlackHole::class
);
$paginator::setCache($cache);

var_dump($paginator->getItemsByPage(1));

Does this work for you? (Please recheck the internal cache ID.)

[weierophinney]

There's another good reason for not using spl_object_hash: it will vary between requests, meaning you'll never get a cache hit on subsequent requests. I think that may have been the root issue behind the original change.

[richard-parnaby-king]

When I use the black hole adapter it appears to work fine. I am using the filesystem cache adapter and I am getting collisions:

$select = new \Zend\Db\Sql\Select('table1'); // add a table name
$dbAdapter = new \Zend\Db\Adapter\Adapter(
        [
    'driver' => '...', // add your database config
    'dsn' => '...',
    'username' => '...',
    'password' => '...',
        ]
);
$adapter = new \Zend\Paginator\Adapter\DbSelect($select, $dbAdapter);
$paginator = new \Zend\Paginator\Paginator($adapter);

$cache = \Zend\Cache\StorageFactory::factory(
                array(
                    'adapter' => array(
                        'name' => 'filesystem',
                        'options' => array(
                            'dirLevel' => 2,
                            'cacheDir' => 'data/cache',
                            'dirPermission' => 0755,
                            'filePermission' => 0666,
                        ),
                    ),
                    'plugins' => array('serializer'),
                )
);
$paginator::setCache($cache);

var_dump($paginator->getItemsByPage(1));

$select = new \Zend\Db\Sql\Select('table2'); // add a table name
$adapter = new \Zend\Paginator\Adapter\DbSelect($select, $dbAdapter);
$paginator = new \Zend\Paginator\Paginator($adapter);

$cache = \Zend\Cache\StorageFactory::factory(
                array(
                    'adapter' => array(
                        'name' => 'filesystem',
                        'options' => array(
                            'dirLevel' => 2,
                            'cacheDir' => 'data/cache',
                            'dirPermission' => 0755,
                            'filePermission' => 0666,
                        ),
                    ),
                    'plugins' => array('serializer'),
                )
);
$paginator::setCache($cache);

var_dump($paginator->getItemsByPage(1));

[mariojrrc]

same problem as reported by @richard-parnaby-king .

[richard-parnaby-king]

I think I have found a solution, however I cannot run the unit tests - I am getting a fatal error:

Fatal error: Class 'ZendTest\Paginator\TestAsset\TestArrayAggregate' not found in C:\zend-paginator\test\FactoryTest.php on line 67

I suspect the autoload generated by composer is incomplete, but I don't know how to fix it.

Could someone:
a) help me get the unit tests working
and/or
b) verify that my change (https://github.com/zendframework/zend-paginator/compare/master...richard-parnaby-king:f60e16f59513a5cb921567e4d8233c96883902c0?expand=1) works so I can do a pull request?

[rcapile]

@richard-parnaby-king your solution would have the same problem of using spl_object_hash, It has an Uptime value for mysql that could change on subsequent requests.

I think the solution for Adapter\DbSelect would be using the SQL itself

protected function _getCacheInternalId()
{
    $adapter = $this->getAdapter();

    if ($adapter instanceof \Zend\Paginator\Adapter\DbSelect) {
        $reflection = new \ReflectionObject($adapter);
        $property = $reflection->getProperty('select');
        $property->setAccessible(true);
        $select = $property->getValue($adapter);
        return md5(
            $reflection->getName()
            . hash('sha512', $select->getSqlString())
            . $this->getItemCountPerPage()
        );
    }

    // @codingStandardsIgnoreEnd
    return md5(
        get_class($adapter)
        . json_encode($adapter)
        . $this->getItemCountPerPage()
    );
}

or make the changes in the Adapter\DbSelect to avoid the Reflection

@Zend\Paginator\Paginator
protected function _getCacheInternalId()
{
    $adapter = $this->getAdapter();

    if ($adapter instanceof \Zend\Paginator\Adapter\DbSelect) {
        return md5(
            get_class($adapter)
            . $adapter->getCacheId()
            . $this->getItemCountPerPage()
        }

    // @codingStandardsIgnoreEnd
    return md5(
        get_class($adapter)
        . json_encode($adapter)
        . $this->getItemCountPerPage()
    );
}

@Zend\Paginator\Adapter\DbSelect
public function getCacheId()
{
    return hash('sha512', $this->select->getSqlString())
}

I could change the interface, but that probably would break someone

@froschdesign what do you think? making a reflection is too ugly?

[richard-parnaby-king]

@rcapile I've been deving on an mssql box so wasn't getting an uptime value. As such, I was getting the same value between requests.

I agree with using the sql string as the source of the hash.

[rcapile]

@richard-parnaby-king I waiting for @froschdesign input to make a pull request.

Although I think the right solution should be changing the Zend\Paginator\Adapter\AdapterInterface but that would be a major BC.

we (we = @mariojrrc ) could make two pull request: one with the code above for the current version and another changing the interface for the next

[froschdesign]

@rcapile

Although I think the right solution should be changing the Zend\Paginator\Adapter\AdapterInterface but that would be a major BC.

I see two problems here:

1. Does the adapter need to know that the result is being cached? No.
2. Changing the interface is a BC break.

[rcapile]

@froschdesign So the reflection is the best way? ou creating a public function getSelect() in DbSelect?

[froschdesign]

I'm really sorry about the late response.

Here is a simple idea:

class DbSelect implements AdapterInterface, JsonSerializable
{
    // ...

    public function jsonSerialize()
    {
        return [
            'select'       => $this->sql->buildSqlString($this->select),
            'count_select' => $this->sql->buildSqlString($this->getSelectCount()),
        ];
    }
}

(Edit: The select query for counting should also included.
Edit 2: The method should not return a hashed value, this should be done in the cache method.)