Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Made the OFFSET in LIMIT queries non-nullable integer defaulting to 0 #3248

Merged
merged 1 commit into from
Aug 12, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions UPGRADE.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
# Upgrade to 3.0

## BC BREAK: The `NULL` value of `$offset` in LIMIT queries is not allowed

The `NULL` value of the `$offset` argument in `AbstractPlatform::(do)?ModifyLimitQuery()` methods is no longer allowed. The absence of the offset should be indicated with a `0` which is now the default value.

## BC BREAK: Removed dbal:import CLI command

The `dbal:import` CLI command has been removed since it only worked with PDO-based drivers by relying on a non-documented behavior of the extension, and it was impossible to make it work with other drivers.
Expand Down
26 changes: 4 additions & 22 deletions lib/Doctrine/DBAL/Platforms/AbstractPlatform.php
Original file line number Diff line number Diff line change
Expand Up @@ -3385,22 +3385,10 @@ public function getTimeFormatString()
/**
* Adds an driver-specific LIMIT clause to the query.
*
* @param string $query
* @param int|null $limit
* @param int|null $offset
*
* @return string
*
* @throws DBALException
*/
final public function modifyLimitQuery($query, $limit, $offset = null)
final public function modifyLimitQuery(string $query, ?int $limit, int $offset = 0) : string
{
if ($limit !== null) {
$limit = (int) $limit;
}

$offset = (int) $offset;

if ($offset < 0) {
throw new DBALException(sprintf(
'Offset must be a positive integer or zero, %d given',
Expand All @@ -3420,21 +3408,15 @@ final public function modifyLimitQuery($query, $limit, $offset = null)

/**
* Adds an platform-specific LIMIT clause to the query.
*
* @param string $query
* @param int|null $limit
* @param int|null $offset
*
* @return string
*/
protected function doModifyLimitQuery($query, $limit, $offset)
protected function doModifyLimitQuery(string $query, ?int $limit, int $offset) : string
{
if ($limit !== null) {
$query .= ' LIMIT ' . $limit;
$query .= sprintf(' LIMIT %d', $limit);
}

if ($offset > 0) {
$query .= ' OFFSET ' . $offset;
$query .= sprintf(' OFFSET %d', $offset);
}

return $query;
Expand Down
2 changes: 1 addition & 1 deletion lib/Doctrine/DBAL/Platforms/DB2Platform.php
Original file line number Diff line number Diff line change
Expand Up @@ -782,7 +782,7 @@ public function getTemporaryTableName($tableName)
/**
* {@inheritDoc}
*/
protected function doModifyLimitQuery($query, $limit, $offset = null)
protected function doModifyLimitQuery(string $query, ?int $limit, int $offset) : string
{
$where = [];

Expand Down
2 changes: 1 addition & 1 deletion lib/Doctrine/DBAL/Platforms/MySqlPlatform.php
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ class MySqlPlatform extends AbstractPlatform
/**
* {@inheritDoc}
*/
protected function doModifyLimitQuery($query, $limit, $offset)
protected function doModifyLimitQuery(string $query, ?int $limit, int $offset) : string
{
if ($limit !== null) {
$query .= ' LIMIT ' . $limit;
Expand Down
2 changes: 1 addition & 1 deletion lib/Doctrine/DBAL/Platforms/OraclePlatform.php
Original file line number Diff line number Diff line change
Expand Up @@ -960,7 +960,7 @@ public function getName()
/**
* {@inheritDoc}
*/
protected function doModifyLimitQuery($query, $limit, $offset = null)
protected function doModifyLimitQuery(string $query, ?int $limit, int $offset) : string
{
if ($limit === null && $offset <= 0) {
return $query;
Expand Down
6 changes: 3 additions & 3 deletions lib/Doctrine/DBAL/Platforms/SQLAnywherePlatform.php
Original file line number Diff line number Diff line change
Expand Up @@ -1333,16 +1333,16 @@ protected function _getTransactionIsolationLevelSQL($level)
/**
* {@inheritdoc}
*/
protected function doModifyLimitQuery($query, $limit, $offset)
protected function doModifyLimitQuery(string $query, ?int $limit, int $offset) : string
{
$limitOffsetClause = '';

if ($limit > 0) {
if ($limit !== null) {
$limitOffsetClause = 'TOP ' . $limit . ' ';
}

if ($offset > 0) {
if ($limit == 0) {
if ($limit === null) {
$limitOffsetClause = 'TOP ALL ';
}

Expand Down
11 changes: 4 additions & 7 deletions lib/Doctrine/DBAL/Platforms/SQLServer2012Platform.php
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
use const PREG_OFFSET_CAPTURE;
use function preg_match;
use function preg_match_all;
use function sprintf;
use function substr_count;

/**
Expand Down Expand Up @@ -94,7 +95,7 @@ protected function getReservedKeywordsClass()
/**
* {@inheritdoc}
*/
protected function doModifyLimitQuery($query, $limit, $offset = null)
protected function doModifyLimitQuery(string $query, ?int $limit, int $offset) : string
{
if ($limit === null && $offset <= 0) {
return $query;
Expand Down Expand Up @@ -127,17 +128,13 @@ protected function doModifyLimitQuery($query, $limit, $offset = null)
}
}

if ($offset === null) {
$offset = 0;
}

// This looks somewhat like MYSQL, but limit/offset are in inverse positions
// Supposedly SQL:2008 core standard.
// Per TSQL spec, FETCH NEXT n ROWS ONLY is not valid without OFFSET n ROWS.
$query .= " OFFSET " . (int) $offset . " ROWS";
$query .= sprintf(' OFFSET %d ROWS', $offset);

if ($limit !== null) {
$query .= " FETCH NEXT " . (int) $limit . " ROWS ONLY";
$query .= sprintf(' FETCH NEXT %d ROWS ONLY', $limit);
}

return $query;
Expand Down
2 changes: 1 addition & 1 deletion lib/Doctrine/DBAL/Platforms/SQLServerPlatform.php
Original file line number Diff line number Diff line change
Expand Up @@ -1229,7 +1229,7 @@ public function getBooleanTypeDeclarationSQL(array $field)
/**
* {@inheritDoc}
*/
protected function doModifyLimitQuery($query, $limit, $offset = null)
protected function doModifyLimitQuery(string $query, ?int $limit, int $offset) : string
{
$where = [];

Expand Down
4 changes: 2 additions & 2 deletions lib/Doctrine/DBAL/Platforms/SqlitePlatform.php
Original file line number Diff line number Diff line change
Expand Up @@ -692,10 +692,10 @@ protected function getPostAlterTableIndexForeignKeySQL(TableDiff $diff)
/**
* {@inheritDoc}
*/
protected function doModifyLimitQuery($query, $limit, $offset)
protected function doModifyLimitQuery(string $query, ?int $limit, int $offset) : string
{
if ($limit === null && $offset > 0) {
return $query . ' LIMIT -1 OFFSET ' . $offset;
$limit = -1;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not strictly related to the issue, just a code duplication cleanup.

}

return parent::doModifyLimitQuery($query, $limit, $offset);
Expand Down
2 changes: 1 addition & 1 deletion tests/Doctrine/Tests/DBAL/Platforms/DB2PlatformTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -354,7 +354,7 @@ public function testModifiesLimitQuery()
{
self::assertEquals(
'SELECT * FROM user',
$this->_platform->modifyLimitQuery('SELECT * FROM user', null, null)
$this->_platform->modifyLimitQuery('SELECT * FROM user', null, 0)
);

self::assertEquals(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -774,7 +774,7 @@ public function testModifiesLimitQueryWithOffset()
);
self::assertEquals(
'SELECT TOP ALL START AT 6 * FROM user',
$this->_platform->modifyLimitQuery('SELECT * FROM user', 0, 5)
$this->_platform->modifyLimitQuery('SELECT * FROM user', null, 5)
);
}

Expand Down
4 changes: 2 additions & 2 deletions tests/Doctrine/Tests/DBAL/Platforms/SQLServerPlatformTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -54,15 +54,15 @@ public function getModifyLimitQueries()
[
'SELECT id_0, MIN(sclr_2) AS dctrn_minrownum FROM (SELECT c0_.id AS id_0, c0_.title AS title_1, ROW_NUMBER() OVER(ORDER BY c0_.title ASC) AS sclr_2 FROM TestTable c0_ ORDER BY c0_.title ASC) dctrn_result GROUP BY id_0 ORDER BY dctrn_minrownum ASC',
30,
null,
0,
'WITH dctrn_cte AS (SELECT TOP 30 id_0, MIN(sclr_2) AS dctrn_minrownum FROM (SELECT c0_.id AS id_0, c0_.title AS title_1, ROW_NUMBER() OVER(ORDER BY c0_.title ASC) AS sclr_2 FROM TestTable c0_) dctrn_result GROUP BY id_0 ORDER BY dctrn_minrownum ASC) SELECT * FROM (SELECT *, ROW_NUMBER() OVER (ORDER BY (SELECT 0)) AS doctrine_rownum FROM dctrn_cte) AS doctrine_tbl WHERE doctrine_rownum <= 30 ORDER BY doctrine_rownum ASC',
],

// Test re-ordered query with no scrubbed ORDER BY clause
[
'SELECT id_0, MIN(sclr_2) AS dctrn_minrownum FROM (SELECT c0_.id AS id_0, c0_.title AS title_1, ROW_NUMBER() OVER(ORDER BY c0_.title ASC) AS sclr_2 FROM TestTable c0_) dctrn_result GROUP BY id_0 ORDER BY dctrn_minrownum ASC',
30,
null,
0,
'WITH dctrn_cte AS (SELECT TOP 30 id_0, MIN(sclr_2) AS dctrn_minrownum FROM (SELECT c0_.id AS id_0, c0_.title AS title_1, ROW_NUMBER() OVER(ORDER BY c0_.title ASC) AS sclr_2 FROM TestTable c0_) dctrn_result GROUP BY id_0 ORDER BY dctrn_minrownum ASC) SELECT * FROM (SELECT *, ROW_NUMBER() OVER (ORDER BY (SELECT 0)) AS doctrine_rownum FROM dctrn_cte) AS doctrine_tbl WHERE doctrine_rownum <= 30 ORDER BY doctrine_rownum ASC',
],
];
Expand Down