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

Handle arbitrary whitespaces when parsing SQL in order to apply LIMIT for MS SQL Server #2372

Merged
merged 1 commit into from
Sep 9, 2016

Conversation

morozov
Copy link
Member

@morozov morozov commented Apr 21, 2016

Given that the DBAL and QueryBuilder allow using plain SQL queries or their chunks, the DBAL should properly parse queries containing line breaks or other arbitrary formatting. Currently SQLServerPlatform::doModifyLimitQuery() (and SQLServer2012Platform::doModifyLimitQuery() which is not released yet) produce invalid SQL in case if the original query contains line breaks or other whitespaces.

@@ -115,12 +115,16 @@ protected function doModifyLimitQuery($query, $limit, $offset = null)

// Queries using OFFSET... FETCH MUST have an ORDER BY clause
// Find the position of the last instance of ORDER BY and ensure it is not within a parenthetical statement
$orderByPos = strripos($query, " ORDER BY ");

if (preg_match_all('/\s+ORDER\s+BY\s+/i', $query, $matches, PREG_OFFSET_CAPTURE)) {
Copy link
Member

Choose a reason for hiding this comment

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

Why PREG_OFFSET_CAPTURE?

Copy link
Member Author

Choose a reason for hiding this comment

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

See the line below. The $orderByPos is obtained from the captured offset.

@billschaller
Copy link
Member

@morozov Does the test suite pass?

Logic change looks fine to me but 😩 @ SQL Server...

@deeky666 deeky666 added this to the 2.5.5 milestone May 26, 2016
@morozov
Copy link
Member Author

morozov commented May 27, 2016

@zeroedin-bill I only ran the new test locally, while at the moment I submitted this PR, the whole build on Travis was far from stable.

@deeky666
Copy link
Member

deeky666 commented Jun 3, 2016

@morozov thanks!

@deeky666 deeky666 modified the milestones: 2.6, 2.5.5 Jun 3, 2016
@deeky666 deeky666 self-assigned this Jun 3, 2016
@deeky666
Copy link
Member

deeky666 commented Jun 3, 2016

Decided to target this PR for 2.6 as the "improved" limit SQL for SQL Server 2012 is not available in 2.5.

@morozov
Copy link
Member Author

morozov commented Sep 9, 2016

Rebased and resolved merge conflicts with #2428.

@Ocramius Ocramius added the Bug label Sep 9, 2016
@Ocramius Ocramius assigned Ocramius and unassigned deeky666 and billschaller Sep 9, 2016
@Ocramius
Copy link
Member

Ocramius commented Sep 9, 2016

This goes in! Thanks @morozov!

@Ocramius Ocramius merged commit ec8fc16 into doctrine:master Sep 9, 2016
@morozov morozov deleted the sql-server-limit branch September 9, 2016 21:46
@Ocramius Ocramius changed the title Handle arbitrary whitespaces when parsing SQL in order to apply LIMIT for MS SQL Server Handle arbitrary whitespaces when parsing SQL in order to apply LIMIT for MS SQL Server Jul 22, 2017
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants