Skip to content

Lower case "order by" keyword causes wrong LIMIT query on SQL Server#563

Merged
deeky666 merged 1 commit intodoctrine:masterfrom
stchr:master
Apr 24, 2014
Merged

Lower case "order by" keyword causes wrong LIMIT query on SQL Server#563
deeky666 merged 1 commit intodoctrine:masterfrom
stchr:master

Conversation

@stchr
Copy link

@stchr stchr commented Apr 2, 2014

SQLServerPlatform::modifyLimitQuery('SELECT * FROM user order by username') (lowercase order by)
returns
SELECT * FROM (SELECT *, ROW_NUMBER() OVER (ORDER BY order) AS doctrine_rownum FROM user order by username) AS doctrine_tbl WHERE doctrine_rownum BETWEEN 1 AND 10
instead of
SELECT * FROM (SELECT *, ROW_NUMBER() OVER (ORDER BY username) AS doctrine_rownum FROM user) AS doctrine_tbl WHERE doctrine_rownum BETWEEN 1 AND 10

@doctrinebot
Copy link

Hello,

thank you for creating this pull request. I have automatically opened an issue
on our Jira Bug Tracker for you. See the issue link:

http://www.doctrine-project.org/jira/browse/DBAL-862

We use Jira to track the state of pull requests and the versions they got
included in.

@Ocramius
Copy link
Member

Ocramius commented Apr 2, 2014

I just noticed it, but the regex doesn't seem to enforce at least one whitespace before the ORDER BY clause

@stchr
Copy link
Author

stchr commented Apr 2, 2014

The regex in line 1153 already enforces one whitespace before ORDER BY.
If you also enforce a whitespace in the regex in line 1172, it will break. Maybe the stristr($query, ' ORDER BY') in line 1152 may get an prefixed whitespace

@Ocramius
Copy link
Member

Ocramius commented Apr 2, 2014

@stchr yeah, re-read it and it seems like it's indeed ok.

@Ocramius
Copy link
Member

Ocramius commented Apr 2, 2014

@deeky666 mergeable

@deeky666
Copy link
Member

deeky666 commented Apr 2, 2014

looks good to me

deeky666 added a commit that referenced this pull request Apr 24, 2014
Lower case "order by" keyword causes wrong LIMIT query on SQL Server
@deeky666 deeky666 merged commit d6b9140 into doctrine:master Apr 24, 2014
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants