Skip to content

SQL Server Paginator bug fix #383

Merged
merged 4 commits into from Oct 1, 2013

4 participants

@flip111
flip111 commented Sep 30, 2013

This patch solves bug http://www.doctrine-project.org/jira/browse/DDC-2687
and takes inspiration from this PR #374 which was written for this bug http://www.doctrine-project.org/jira/browse/DDC-2622

@doctrinebot

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-620

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

@flip111
flip111 commented Sep 30, 2013

To be improved:
1. $isWrapped = (preg_match('/SELECT DISTINCT .* FROM (.) dctrn_result/', $query)) ? true : false;
2. review /%s.%s\s+(AS\s+)?([^,\s)]+) and /%s.(%s)\s
(AS)?\s([^,\s)]) to see if they can be the same, then de-double them.

@flip111 flip111 Made cleaner code and remove unneccessary checks.
1. combine the regexes into one. The only thing that needs to be captured
is the alias. Used a non-capturing group for "AS " to avoid confusion and
have cleaner regex. It tries to match "Table.Column Alias" or
"Table.Column AS Alias". (line 861)
2. No fallback on line 865 like "$column['table'] . '.' .
$column['column']" this wouldn't make sense as this can not be possible
when it's a wrapped query. Better not insert some random SQL that's gonna
confuse the hell out of everybody.
3. Removed "($column['hasTable'] ? $column['table']  . '.' : '') .
$column['column']" because the original regex always tries to match the
table name. Therefor hasTable will always be true or the entire function
fails anyway. (this references line 868 now) (this was unit tested on
mysql, postgresql and sql server and did not increase errors/failures)
75048b2
@flip111
flip111 commented Oct 1, 2013

Forgot to run the DBAL test suite (only did ORM) :(

@flip111
flip111 commented Oct 1, 2013

Why is there an SQL Server platform test on a PostgreSQL platform ? https://travis-ci.org/doctrine/dbal/jobs/12001500 Same problem as here #376

@stof
Doctrine member
stof commented Oct 1, 2013

@flip111 Functional tests (which call the actual DB) are run using the configured platform. Unit tests (which don't connect to the database) are run without taking into account the platform for which you run your functional tests, so that these ones are always run, even if you don't have SQL Server locally (because they don't need SQL Server to run)

@flip111
flip111 commented Oct 1, 2013

Where do you think there should be code inserted to skip the tests for platforms that do not (yet) implement these methods?

@flip111 flip111 Put back some old code and fixed test case.
Apperently the doModifyLimitQuery function does not only cover SQL
generated by ORM (Makes sense to use DBAL as seperate component). Therefor
it can happen that a column name does not include the table and so the old
logic was valid anyway. (line 867-869)

Fixed the unit tests to use real column names instead of aliases.
c6b4ff2
@guilhermeblanco guilhermeblanco merged commit b4bcc18 into doctrine:master Oct 1, 2013

1 check passed

Details default The Travis CI build passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.