Fixes #5642 #5643

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
6 participants
Contributor

samsonasik commented Dec 24, 2013

Fixes #5642

Contributor

macnibblet commented Dec 24, 2013

Adding a group annotation on the test with the issue id is never a bad idea

Contributor

samsonasik commented Dec 24, 2013

@macnibblet a group annotation added ;)

Contributor

macnibblet commented Dec 24, 2013

i think the annotation must be added to the method, but i could be wrong.

Contributor

samsonasik commented Dec 24, 2013

@macnibblet I'm not sure if it placed on the method because so many tests on one method. should I reset last commit ( a group annotation addition ) ?

YonmaN commented Dec 25, 2013

Looks spot-on, many thanks

+ if ($this->offset === null) {
+ return null;
+ }
+ $this->limit = "18446744073709551615"; //maximum of unsigned BIGINT
@samsonasik

samsonasik Dec 27, 2013

Contributor

@ralphschindler about platform specific, should it changed by :

if ($platform instanceof \Zend\Db\Sql\Platform\Mysql && $this->offset !== null) {
    $this->limit =  "18446744073709551615"; //maximum of unsigned BIGINT
}
return null;

is it better ?

@turrsis

turrsis Jan 5, 2014

Contributor

for MySql this will be overvritten by Zend\Db\Sql\Platform\Mysql\SelectDecorator
as i know - postgresql allow use offset withot limit
as i understand - this is MySql issue - and should be fixed in Zend\Db\Sql\Platform\Mysql\SelectDecorator::processLimit

@samsonasik

samsonasik Jan 5, 2014

Contributor

@turrsis I think no, it won't be overwritten, for example :

$table = 'users';
$sampleTable = new TableGateway($table, $adapter, null,new HydratingResultSet());
$select = new Select($table);
$select->offset(10);
echo $select->getSqlString(new \Zend\Db\Adapter\Platform\Mysql);

it will get

SELECT `users`.* FROM `users` LIMIT 18446744073709551615 OFFSET 10

what case it will be overwritten with Zend\Db\Sql\Platform\Mysql\SelectDecorator ? or should I add the condition at SelectDecorator too ?

@turrsis

turrsis Jan 5, 2014

Contributor
        $mockDriver = $this->getMock('Zend\Db\Adapter\Driver\DriverInterface');
        $mockConnection = $this->getMock('Zend\Db\Adapter\Driver\ConnectionInterface');
        $mockDriver->expects($this->any())->method('checkEnvironment')->will($this->returnValue(true));
        $mockDriver->expects($this->any())->method('getConnection')->will($this->returnValue($mockConnection));
        $mockPlatform = $this->getMock('Zend\Db\Adapter\Platform\PlatformInterface');
        $mockStatement = $this->getMock('Zend\Db\Adapter\Driver\StatementInterface');
        $mockDriver->expects($this->any())->method('createStatement')->will($this->returnValue($mockStatement));

        $adapter = new \Zend\Db\Adapter\Adapter($mockDriver, $mockPlatform);

        $sql = new \Zend\Db\Sql\Sql($adapter);
        $sql->getSqlPlatform()->setTypeDecorator('Zend\Db\Sql\Select', new \Zend\Db\Sql\Platform\Mysql\SelectDecorator);
        $query = $sql->getSqlStringForSqlObject($select46, new TrustingSql92Platform());

this code (for unitTests) use Zend\Db\Sql\Platform\Mysql\SelectDecorator::processLimit

if you code not use decorators - this is the bug, see this zendframework#5306 - this fix for decorators, subqueries, etc.

@ralphschindler are you planing to merge zendframework#5306 ?

@turrsis

turrsis Jan 5, 2014

Contributor

if Decorator exist - you should use it and only it, otherwise it is a bug

@ralphschindler

ralphschindler Mar 7, 2014

Member

Did you plan to move this logic to the Mysql Select decorator?

@samsonasik

samsonasik Mar 7, 2014

Contributor

I think the solution is to copy instead of move, because we can use new Select instead as I described above, is it ok ? @ralphschindler

@ralphschindler

ralphschindler Mar 7, 2014

Member

I don't think so, paging in database adapters, across the board, is already a non-standard thing. Seeing as though this is strongly a mysqlism, I think it makes the most sense to do this in the MySQL decorator. For it to be in Select, there would have to be a VERY strong argument that this particular workflow applies to most of the other popular DB platforms: postgres, sqlite, SQL Server, Db2 and Oracle.

@samsonasik

samsonasik Mar 7, 2014

Contributor

[update] I will try, but personally I don't agree to cast $limit with (int) because on some cases like this , we need big limit.... ( 18446744073709551615 ) which is not (int)

@samsonasik

samsonasik Mar 7, 2014

Contributor

@ralphschindler I have tried to move it to selectdecorator, and update SelectDecoratorTest.php, but it comes with error :

Attempting to quote a value in Zend\Db\Adapter\Platform\Mysql without extension/driver support can introduce security vulnerabilities in a production environment.

because "18446744073709551615" is quoted. because it using MysqlPlatform instead of TrustingSql92Platform, any idea ?

@ghost ghost assigned ralphschindler Jan 2, 2014

@ralphschindler ralphschindler added the Db label Feb 27, 2014

Owner

weierophinney commented Mar 3, 2014

@ralphschindler Please set a milestone.

@ralphschindler ralphschindler added this to the 2.3.0 milestone Mar 4, 2014

Member

ralphschindler commented Mar 10, 2014

@samsonasik please see #5940

@samsonasik samsonasik closed this Mar 10, 2014

weierophinney added a commit that referenced this pull request Mar 10, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment