[DDC-2310] [DDC-2675] Fix SQL generation on table lock hint capable platforms

[deeky666]

SQL Server and SQL Anywhere use table lock hints for locking. ORM generates wrong SQL when used in conjunction with joins, in a table inheritance scenario or in a subquery. It places the table lock hints after the JOIN clauses, which is wrong. The table lock hints have to be placed after each table alias in the FROM clause.

Example (current)

SELECT t0.id FROM users t0 INNER JOIN profiles t1 ON t0.id = t1.user_id WITH (NOLOCK)

Example (expected)

SELECT t0.id FROM users t0 WITH (NOLOCK) INNER JOIN profiles t1 ON t0.id = t1.user_id

The testsuites fail big time at the moment because of that and I suppose ORM is more or less unusable at the moment for those platforms without this patch.

I needed to adjust a lot of tests which compared compiled DQL, to make use of the platforms specific lock hint clauses.
Also I did not add new tests because there are already so many tests, where this fails without this patch that this is not necessary IMO.

[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/DDC-2914

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

[billschaller]

Thank you, from the bottom of my heart. I was just checking back on this today because it came up again in development. I had just forked DBAL and commented out the line that appends the lock hint.

[deeky666]

Just for reference: This issue was partly introduced in doctrine/dbal#257 but that PR was not the root cause of the problem because the proper SQL generation was broken ever since for this.
@zeroedin-bill Sorry for the delay on this issue, I have been pointed into this direction yesterday for the first time :(

[billschaller]

@deeky666 no worries, it was easy enough to work around in the interim :)

[guilhermeblanco]

@deeky666 seems perfect to me! Once travis says yay, I'll merge it. =)

[deeky666]

@guilhermeblanco IMO this should be backported to the 2.3 and 2.4 branches also because it breaks nearly all queries ;)
Also I'm not even entirely sure whether the current DBAL implementation of SQLServerPlatform::appendLockHint() is correct. It seems ORM passes null to that method if no query hint is set and the method evaluates this to WITH (NOLOCK). I wonder if we should do a strict comparison for 0 (no lock) there and do not add a hint if null is given. What do you think?

[billschaller]

@deeky666 That's a good point -- SQL server default is transaction level READ COMMITTED, but using WITH (NOLOCK) is the equivalent of using SET TRANSACTION LEVEL READ UNCOMMITTED. This should really not append a lock hint unless explicitly requested.

[deeky666]

@zeroedin-bill This is a good argument. Okay then I will change that in DBAL, too.
@guilhermeblanco Please wait with merge until the DBAL fix is ready because I will have to change some test here again afterwards.

[deeky666]

@zeroedin-bill @guilhermeblanco DBAL patch supplied in doctrine/dbal#508.

[Ocramius]

int|null I suppose

[deeky666]

Not yet ;) This will be changed in the PR for DDC-2919.

[deeky666]

@Ocramius It's ready now from my perspective. Your remaining remarks will be implemented in the follow-up PR :)

[Ocramius]

👍

[deeky666]

@beberlei @guilhermeblanco Is this patch okay for you? As soon as you are ok and merge I can continue on the followup DDC-2919 for further SQL Server and SQL Anywhere fixes :)

[flip111]

@zeroedin-bill good advice about TRANSACTION LEVEL 👍

[billschaller]

Very glad to see this getting merged!