Skip to content
This repository has been archived by the owner on Jan 8, 2020. It is now read-only.

Zend\Db\Sql Allow MySQL to use limit when only offset was provided #5940

Merged
merged 4 commits into from
Mar 10, 2014
Merged

Zend\Db\Sql Allow MySQL to use limit when only offset was provided #5940

merged 4 commits into from
Mar 10, 2014

Conversation

ralphschindler
Copy link
Member

This is a fix for #5642
And would supersede PR #5643

@samsonasik
Copy link
Contributor

@ralphschindler it still doesn't work if I do :

$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);

I tried to test it in Zend\Db\Sql\SelectTest.php with

$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('?')
        );

and I got :

--- Expected
+++ Actual
@@ @@
-'SELECT "foo".* FROM "foo" LIMIT '18446744073709551615' OFFSET '10''
+'SELECT "foo".* FROM "foo" OFFSET '10''

@ralphschindler
Copy link
Member Author

That is correct. You need to use the Zend\Db\Sql\Sql object to get a platform specific SQL string, see the original issue. He is using the SQL object to prepare a statement customized for the mysql platform. So your example would more likely be:

use Zend\Db\Sql;
$sql = new Sql\Sql;
$select = $sql->select($table);
$select->offset(10);
echo $sql->getSqlStringForSqlObject($select);

@samsonasik
Copy link
Contributor

so, using getSqlString is wrong way ?

@samsonasik
Copy link
Contributor

the getSqlString passed the platform too (Zend\Db\Adapter\Platform\PlatformInterface) :

echo $select->getSqlString(new \Zend\Db\Adapter\Platform\Mysql);

@ralphschindler
Copy link
Member Author

echo $select->getSqlString(new \Zend\Db\Adapter\Platform\Mysql);

Will produce a SQL string with the proper quoting/adapter abstraction, but not with the proper SQL abstraction applied. For SQL language abstraction, then Zend\Db\Sql\Sql should be used.

@samsonasik
Copy link
Contributor

Ok, but I tried to use big offset to test yours in ZendTest/Db/Sql/Platform/Mysql/SelectDecoratorTest.php with :

        $select2 = new Select;
        $select2->from('foo')->limit(5)->offset(10000000000000000000);
        $expectedPrepareSql2 = 'SELECT `foo`.* FROM `foo` LIMIT ? OFFSET ?';
        $expectedParams2 = array('offset' => 10000000000000000000, 'limit' => 5);
        $expectedSql2 = 'SELECT `foo`.* FROM `foo` LIMIT 5 OFFSET 10000000000000000000';
--- Expected
+++ Actual
@@ @@
-'SELECT `foo`.* FROM `foo` LIMIT 5 OFFSET 10000000000000000000'
+'SELECT `foo`.* FROM `foo` LIMIT 5 OFFSET 1.0E+19'

so I think $this->limit and $this->offset should not be cast to (int).

@ralphschindler
Copy link
Member Author

Ok, we can remove the casting.

@ralphschindler
Copy link
Member Author

Ok, I've fixed it and added tests. The problem with your above example is that that number you're passing into offset() is actually not an integer, it will be used as a float as it is outside the maximum allowable integer space in PHP. You'll need to pass it as a string, as I've noted in my tests.

@samsonasik
Copy link
Contributor

@ralphschindler thank you ;)

weierophinney added a commit that referenced this pull request Mar 10, 2014
Zend\Db\Sql Allow MySQL to use limit when only offset was provided
weierophinney added a commit that referenced this pull request Mar 10, 2014
@weierophinney weierophinney merged commit 8ef87ac into zendframework:develop Mar 10, 2014
@samsonasik
Copy link
Contributor

@ralphschindler I created PR #5941 to remove cast to (int) in Zend\Db\Sql\Select.php too for offset and limit.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants