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

Already on GitHub? Sign in to your account

[DDC-2310] Fix evaluation of NOLOCK table hint on SQL Server #508

Merged
merged 1 commit into from Jan 14, 2014

Conversation

Projects
None yet
4 participants
Member

deeky666 commented Jan 13, 2014

This PR is complementary to doctrine/doctrine2#910.
ORM passes null to AbstractPlatform::appendLockHint() as $lockMode which should not evaluated to LockMode::NONE unless 0 is explictly given. Otherwise ORM appends WITH (NOLOCK) to all queries even though, no query lock hint is set.

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

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

@Ocramius Ocramius and 1 other commented on an outdated diff Jan 14, 2014

lib/Doctrine/DBAL/Platforms/SQLServerPlatform.php
@@ -1347,14 +1347,14 @@ public function rollbackSavePoint($savepoint)
*/
public function appendLockHint($fromClause, $lockMode)
{
- switch ($lockMode) {
- case LockMode::NONE:
+ switch (true) {
@Ocramius

Ocramius Jan 14, 2014

Owner

Can you change this to if (...) { return ...; } blocks?

@deeky666

deeky666 Jan 14, 2014

Member

yes I will do.

Owner

Ocramius commented Jan 14, 2014

@deeky666 I'd say the ORM is wrong here, since the docblock for appendLockHint requires an integer to be passed in. Probably action to be taken only on ORM?

Member

deeky666 commented Jan 14, 2014

@Ocramius I don't know if action has to be taken in ORM. At least the current implementation here is wrong because null is not a valid input parameter and should not be considered as explicit NOLOCK.
The question is what to do about invalid input here? Throw exception? This is not in the DocBlock, either =/
In SQLAnywherePlatform::appendLockHint() I threw an exception for unknown lock modes, but that failes big time in ORM, too.

Owner

Ocramius commented Jan 14, 2014

@deeky666 changing the docblock to allow integer|null is also acceptable and better than introducing a BC that requires changes in ORM.

Introducing the exception can be simply if ( ! is_in($lockMode)) { throw ...; }. A bit stupid, but helps identifying where the ORM is doing it wrong immediately. I wouldn't go down this path though.

@beberlei beberlei added a commit that referenced this pull request Jan 14, 2014

@beberlei beberlei Merge pull request #508 from deeky666/fix-sql-server-nolock-hint
[DDC-2310] Fix evaluation of NOLOCK table hint on SQL Server
72eb3a7

@beberlei beberlei merged commit 72eb3a7 into doctrine:master Jan 14, 2014

1 check passed

default The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment