zendframework / zend-db

Db component from Zend Framework
BSD 3-Clause "New" or "Revised" License
101 stars 122 forks source link

Select->getSqlString returning invalid SQL for LIMIT and OFFSET #50

Open hoppithek opened 8 years ago

hoppithek commented 8 years ago

getSqlString quotes the values for LIMIT and OFFSET, which results in int values quoted to strings, which in turn results in a syntax error for example in MySQL. https://github.com/zendframework/zend-db/blob/master/src/Sql/Select.php#L739 https://github.com/zendframework/zend-db/blob/master/src/Sql/Select.php#L752

Very simple test

$sql = @(new \Zend\Db\Sql\Select)->from('tableName')->limit(10)->offset(0)->getSqlString(new \Zend\Db\Adapter\Platform\Mysql);

$sql === "SELECTtableName.* FROMtableNameLIMIT '10' OFFSET '0'" This is a Syntax error for MySQL and possibly others as well.

I tested this with the PDO driver https://github.com/zendframework/zend-db/blob/master/src/Adapter/Platform/Mysql.php#L93 quotes the int values to strings.

Noteworhty

When preparing and executing the same Select as statement the correct sql "SELECTtableName.* FROMtableNameLIMIT 10 OFFSET 0" is generated and send to the MySQL server.

Possible untested solution

Since Select->limit(numeric $value) and Select->offset(numeric $value) only allow numeric values quoting might be omitted, but I cannot say this for sure as quotes might be needed for other platforms or a different use case might require it. https://github.com/zendframework/zend-db/blob/master/src/Sql/Select.php#L360-L366 https://github.com/zendframework/zend-db/blob/master/src/Sql/Select.php#L378-L384

Another alternative is adding quoteLimit($value) and quoteOffset($value) to the platform interface and use them in processLimit and processOffset

larsnystrom commented 8 years ago

The workaround for this is to use Zend\Db\Sql\Sql::buildSqlString() instead of Zend\Db\Sql\Select::getSqlString().

Example:

// $adapter is a Zend\Db\Adapter\AdapterInterface
// $select is a Zend\Db\Sql\Select
$sqlObject = new Zend\Db\Sql\Sql($adapter);
$sqlString = $sqlObject->buildSqlString($select, $adapter);
h3christophe commented 8 years ago

@larsnystrom

This alternative cannot be used when you are using the Zend\Db\Sql\Insert with a Zend\Db\Sql\ISelect This is using mysql adapter

$subSelect = new Select();
$select->from("table");
$select->limit(10)
$select->offset(20)

$insert = new Insert();
$insert->table("insertInto");
$insert->select($select);

Then the generated sql string will quote the limit and offset values which is incorrect

INSERT INTO `insertInto" SELECT * FROM  `table` LIMIT '10' OFFSET '0'

The correct value should be

INSERT INTO `insertInto" SELECT * FROM  `table` LIMIT 10 OFFSET 0

Thank you

h3christophe commented 8 years ago

My solution was to extend the Zend\Db\Sql\Select File and "fixed" the functions

protected function processLimit(PlatformInterface $platform, DriverInterface $driver = null, ParameterContainer $parameterContainer = null)
    {
        if ($this->limit === null) {
            return;
        }
       if ($parameterContainer) {
            $parameterContainer->offsetSet('limit', $this->limit, ParameterContainer::TYPE_INTEGER);
            return [$driver->formatParameterName('limit')];
        }
        return [intval($this->limit)];
        //return [$platform->quoteValue($this->limit)];
    }

    protected function processOffset(PlatformInterface $platform, DriverInterface $driver = null, ParameterContainer $parameterContainer = null)
    {
        if ($this->offset === null) {
            return;
        }
        if ($parameterContainer) {
            $parameterContainer->offsetSet('offset', $this->offset, ParameterContainer::TYPE_INTEGER);
            return [$driver->formatParameterName('offset')];
        }
        return [intval($this->offset)];
        //return [$platform->quoteValue($this->offset)];
    }

I am using

return [intval($this->offset)];

instead of

return [$platform->quoteValue($this->offset)];

I am not sure if this is the way to go though.... i have not tested all the scenarios

bartmcleod commented 5 years ago

Hard to believe that this is still open after 4.5 years. Probably because a workaround exits?

michalbundyra commented 4 years ago

This repository has been closed and moved to laminas/laminas-db; a new issue has been opened at https://github.com/laminas/laminas-db/issues/136.