Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Fixes #5642 #5643

Closed
wants to merge 2 commits into from

6 participants

Abdul Malik Ikhsan Antoine Hedgecock YonmaN Matthew Weier O'Phinney Ralph Schindler Yaroslav Sych
Antoine Hedgecock

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

Abdul Malik Ikhsan

@macnibblet a group annotation added ;)

Antoine Hedgecock

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

Abdul Malik Ikhsan

@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

Looks spot-on, many thanks

Abdul Malik Ikhsan samsonasik commented on the diff
library/Zend/Db/Sql/Select.php
@@ -909,10 +909,13 @@ protected function processOrder(PlatformInterface $platform, DriverInterface $dr
protected function processLimit(PlatformInterface $platform, DriverInterface $driver = null, ParameterContainer $parameterContainer = null)
{
if ($this->limit === null) {
- return null;
+ if ($this->offset === null) {
+ return null;
+ }
+ $this->limit = "18446744073709551615"; //maximum of unsigned BIGINT

@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 ?

Yaroslav Sych
turrsis added a note

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

@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 ?

Yaroslav Sych
turrsis added a note
        $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 #5306 - this fix for decorators, subqueries, etc.

@ralphschindler are you planing to merge #5306 ?

Yaroslav Sych
turrsis added a note

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

Ralph Schindler Collaborator

#5306 is way too big, weren't you going to split that up into smaller PRs?

As for "LIMIT 18446744073709551615 OFFSET 10" is this a common MySQL practice? What are the side effects of this inside the mysql engine?

It seems to me that if if the platform wants both limit and offset, then the developer should ensure that in practice they are supplying both.

@ralphschindler yes, it was documented at MySQL documentation : http://dev.mysql.com/doc/refman/5.0/en/select.html#id4651990 , there is other side, on doctrine case : doctrine/dbal@3f220a1

Ralph Schindler Collaborator

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

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

Ralph Schindler Collaborator

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.

[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)

@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 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Ralph Schindler ralphschindler added the Db label
Matthew Weier O'Phinney

@ralphschindler Please set a milestone.

Ralph Schindler ralphschindler added this to the 2.3.0 milestone
Ralph Schindler
Collaborator

@samsonasik please see #5940

Abdul Malik Ikhsan samsonasik closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Dec 24, 2013
  1. Abdul Malik Ikhsan

    Fixes #5642

    samsonasik authored
  2. Abdul Malik Ikhsan
This page is out of date. Refresh to see the latest.
7 library/Zend/Db/Sql/Select.php
View
@@ -909,10 +909,13 @@ protected function processOrder(PlatformInterface $platform, DriverInterface $dr
protected function processLimit(PlatformInterface $platform, DriverInterface $driver = null, ParameterContainer $parameterContainer = null)
{
if ($this->limit === null) {
- return null;
+ if ($this->offset === null) {
+ return null;
+ }
+ $this->limit = "18446744073709551615"; //maximum of unsigned BIGINT

@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 ?

Yaroslav Sych
turrsis added a note

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

@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 ?

Yaroslav Sych
turrsis added a note
        $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 #5306 - this fix for decorators, subqueries, etc.

@ralphschindler are you planing to merge #5306 ?

Yaroslav Sych
turrsis added a note

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

Ralph Schindler Collaborator

#5306 is way too big, weren't you going to split that up into smaller PRs?

As for "LIMIT 18446744073709551615 OFFSET 10" is this a common MySQL practice? What are the side effects of this inside the mysql engine?

It seems to me that if if the platform wants both limit and offset, then the developer should ensure that in practice they are supplying both.

@ralphschindler yes, it was documented at MySQL documentation : http://dev.mysql.com/doc/refman/5.0/en/select.html#id4651990 , there is other side, on doctrine case : doctrine/dbal@3f220a1

Ralph Schindler Collaborator

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

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

Ralph Schindler Collaborator

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.

[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)

@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 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
}
- $limit = (int) $this->limit;
+ $limit = $this->limit;
if ($driver) {
$sql = $driver->formatParameterName('limit');
16 tests/ZendTest/Db/Sql/SelectTest.php
View
@@ -1158,6 +1158,21 @@ public function providerData()
);
/**
+ * offset without limit
+ * @group ZF2-5642
+ */
+ $select46 = new Select;
+ $select46->from('foo')->offset("10");
+ $sqlPrep46 = 'SELECT "foo".* FROM "foo" LIMIT ? OFFSET ?';
+ $sqlStr46 = 'SELECT "foo".* FROM "foo" LIMIT \'18446744073709551615\' OFFSET \'10\'';
+ $params46 = array('limit' => "18446744073709551615", 'offset' => 10);
+ $internalTests46 = array(
+ 'processSelect' => array(array(array('"foo".*')), '"foo"'),
+ 'processLimit' => array('?'),
+ 'processOffset' => array('?')
+ );
+
+ /**
* $select = the select object
* $sqlPrep = the sql as a result of preparation
* $params = the param container contents result of preparation
@@ -1213,6 +1228,7 @@ public function providerData()
array($select43, $sqlPrep43, array(), $sqlStr43, $internalTests43),
array($select44, $sqlPrep44, array(), $sqlStr44, $internalTests44),
array($select45, $sqlPrep45, $params45, $sqlStr45, $internalTests45),
+ array($select46, $sqlPrep46, $params46, $sqlStr46, $internalTests46),
);
}
}
Something went wrong with that request. Please try again.