Repository: doctrine/dbal (php)

Issue:
Throw exception on invalid LockMode
### Feature Request

|    Q        |   A
|------------ | ------
| New Feature | no
| RFC         | no
| BC Break    | yes

#### Summary

As mentioned in https://github.com/doctrine/dbal/pull/4400#discussion_r522239670, we should throw an exception when passing an invalid `LockMode` to `AbstractPlatform::appendLockHint()`.

Pull request:
Throw exception on invalid LockMode
|      Q       |   A
|------------- | -----------
| Type         | improvement
| BC Break     | yes

Closes #4428.

Patch:
diff --git a/src/Exception/InvalidLockMode.php b/src/Exception/InvalidLockMode.php
new file mode 100644
index 000000000..9a4d6c77b
--- /dev/null
+++ b/src/Exception/InvalidLockMode.php
@@ -0,0 +1,24 @@
+<?php
+
+namespace Doctrine\DBAL\Exception;
+
+use Doctrine\DBAL\Exception;
+
+use function sprintf;
+
+/**
+ * @psalm-immutable
+ */
+class InvalidLockMode extends Exception
+{
+    public static function fromLockMode(int $lockMode): self
+    {
+        return new self(
+            sprintf(
+                'Lock mode %d is invalid. The valid values are LockMode::NONE, LockMode::OPTIMISTIC'
+                    . ', LockMode::PESSIMISTIC_READ and LockMode::PESSIMISTIC_WRITE',
+                $lockMode
+            )
+        );
+    }
+}
diff --git a/src/Platforms/AbstractPlatform.php b/src/Platforms/AbstractPlatform.php
index b0dcef255..abfabdcc6 100644
--- a/src/Platforms/AbstractPlatform.php
+++ b/src/Platforms/AbstractPlatform.php
@@ -13,6 +13,8 @@ use Doctrine\DBAL\Event\SchemaCreateTableEventArgs;
 use Doctrine\DBAL\Event\SchemaDropTableEventArgs;
 use Doctrine\DBAL\Events;
 use Doctrine\DBAL\Exception;
+use Doctrine\DBAL\Exception\InvalidLockMode;
+use Doctrine\DBAL\LockMode;
 use Doctrine\DBAL\Platforms\Keywords\KeywordList;
 use Doctrine\DBAL\Schema\Column;
 use Doctrine\DBAL\Schema\ColumnDiff;
@@ -1304,15 +1306,21 @@ abstract class AbstractPlatform
      * Honors that some SQL vendors such as MsSql use table hints for locking instead of the
      * ANSI SQL FOR UPDATE specification.
      *
-     * @param string   $fromClause The FROM clause to append the hint for the given lock mode to.
-     * @param int|null $lockMode   One of the Doctrine\DBAL\LockMode::* constants. If null is given, nothing will
-     *                             be appended to the FROM clause.
-     *
-     * @return string
+     * @param string $fromClause The FROM clause to append the hint for the given lock mode to
+     * @param int    $lockMode   One of the Doctrine\DBAL\LockMode::* constants
      */
-    public function appendLockHint($fromClause, $lockMode)
+    public function appendLockHint(string $fromClause, int $lockMode): string
     {
-        return $fromClause;
+        switch ($lockMode) {
+            case LockMode::NONE:
+            case LockMode::OPTIMISTIC:
+            case LockMode::PESSIMISTIC_READ:
+            case LockMode::PESSIMISTIC_WRITE:
+                return $fromClause;
+
+            default:
+                throw InvalidLockMode::fromLockMode($lockMode);
+        }
     }
 
     /**
diff --git a/src/Platforms/SQLServer2012Platform.php b/src/Platforms/SQLServer2012Platform.php
index 0571134f3..51e879652 100644
--- a/src/Platforms/SQLServer2012Platform.php
+++ b/src/Platforms/SQLServer2012Platform.php
@@ -2,6 +2,7 @@
 
 namespace Doctrine\DBAL\Platforms;
 
+use Doctrine\DBAL\Exception\InvalidLockMode;
 use Doctrine\DBAL\LockMode;
 use Doctrine\DBAL\Schema\Column;
 use Doctrine\DBAL\Schema\ColumnDiff;
@@ -1535,23 +1536,23 @@ SQL
         return parent::getForeignKeyReferentialActionSQL($action);
     }
 
-    /**
-     * {@inheritDoc}
-     */
-    public function appendLockHint($fromClause, $lockMode)
+    public function appendLockHint(string $fromClause, int $lockMode): string
     {
-        switch (true) {
-            case $lockMode === LockMode::NONE:
+        switch ($lockMode) {
+            case LockMode::NONE:
                 return $fromClause . ' WITH (NOLOCK)';
 
-            case $lockMode === LockMode::PESSIMISTIC_READ:
+            case LockMode::OPTIMISTIC:
+                return $fromClause;
+
+            case LockMode::PESSIMISTIC_READ:
                 return $fromClause . ' WITH (HOLDLOCK, ROWLOCK)';
 
-            case $lockMode === LockMode::PESSIMISTIC_WRITE:
+            case LockMode::PESSIMISTIC_WRITE:
                 return $fromClause . ' WITH (UPDLOCK, ROWLOCK)';
 
             default:
-                return $fromClause;
+                throw InvalidLockMode::fromLockMode($lockMode);
         }
     }
 


Test patch:
diff --git a/tests/Platforms/AbstractPlatformTestCase.php b/tests/Platforms/AbstractPlatformTestCase.php
index 04ca57c02..5a6dab865 100644
--- a/tests/Platforms/AbstractPlatformTestCase.php
+++ b/tests/Platforms/AbstractPlatformTestCase.php
@@ -5,6 +5,7 @@ namespace Doctrine\DBAL\Tests\Platforms;
 use Doctrine\Common\EventManager;
 use Doctrine\DBAL\Events;
 use Doctrine\DBAL\Exception;
+use Doctrine\DBAL\Exception\InvalidLockMode;
 use Doctrine\DBAL\Platforms\AbstractPlatform;
 use Doctrine\DBAL\Platforms\Keywords\KeywordList;
 use Doctrine\DBAL\Schema\Column;
@@ -1477,6 +1478,12 @@ abstract class AbstractPlatformTestCase extends TestCase
             ['CHAR(12)', ['length' => 12, 'fixed' => true]],
         ];
     }
+
+    public function testInvalidLockMode(): void
+    {
+        $this->expectException(InvalidLockMode::class);
+        $this->platform->appendLockHint('TABLE', 128);
+    }
 }
 
 interface GetCreateTableSqlDispatchEventListener
diff --git a/tests/Platforms/SQLServerPlatformTest.php b/tests/Platforms/SQLServerPlatformTest.php
index 303a2804a..7e6925ebf 100644
--- a/tests/Platforms/SQLServerPlatformTest.php
+++ b/tests/Platforms/SQLServerPlatformTest.php
@@ -16,7 +16,7 @@ class SQLServerPlatformTest extends AbstractSQLServerPlatformTestCase
     /**
      * @dataProvider getLockHints
      */
-    public function testAppendsLockHint(?int $lockMode, string $lockHint): void
+    public function testAppendsLockHint(int $lockMode, string $lockHint): void
     {
         $fromClause     = 'FROM users';
         $expectedResult = $fromClause . $lockHint;
@@ -30,7 +30,6 @@ class SQLServerPlatformTest extends AbstractSQLServerPlatformTestCase
     public static function getLockHints(): iterable
     {
         return [
-            [null, ''],
             [LockMode::NONE, ' WITH (NOLOCK)'],
             [LockMode::OPTIMISTIC, ''],
             [LockMode::PESSIMISTIC_READ, ' WITH (HOLDLOCK, ROWLOCK)'],