bug #38986 [DoctrineBridge] accept converting Uid-as-strings to db-values (nicolas-grekas)

fabpot · fabpot · commit dc0d45d31ad5 · 2020-11-05T09:48:15.000+01:00
This PR was merged into the 5.2-dev branch. Discussion ---------- [DoctrineBridge] accept converting Uid-as-strings to db-values | Q | A | ------------- | --- | Branch? | 5.2 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | Fix #38929 | License | MIT | Doc PR | - In #38605 I made Uid types stricter by taking inspiration from native Doctrine types. But #38929 (comment) made me realize this doesn't work with ParamConverters. Here is the fix. Commits ------- 20714d6 [DoctrineBridge] accept converting Uid-as-strings to db-values
diff --git a/src/Symfony/Bridge/Doctrine/Tests/Types/UlidBinaryTypeTest.php b/src/Symfony/Bridge/Doctrine/Tests/Types/UlidBinaryTypeTest.php
@@ -52,11 +52,12 @@ public function testUlidConvertsToDatabaseValue()
         $this->assertEquals($expected, $actual);
     }
 
-    public function testNotSupportedStringUlidConversionToDatabaseValue()
+    public function testStringUlidConvertsToDatabaseValue()
     {
-        $this->expectException(ConversionException::class);
+        $expected = Ulid::fromString(self::DUMMY_ULID)->toBinary();
+        $actual = $this->type->convertToDatabaseValue(self::DUMMY_ULID, $this->platform);
 
-        $this->type->convertToDatabaseValue(self::DUMMY_ULID, $this->platform);
+        $this->assertEquals($expected, $actual);
     }
 
     public function testNotSupportedTypeConversionForDatabaseValue()
diff --git a/src/Symfony/Bridge/Doctrine/Tests/Types/UlidTypeTest.php b/src/Symfony/Bridge/Doctrine/Tests/Types/UlidTypeTest.php
@@ -68,11 +68,14 @@ public function testUlidInterfaceConvertsToDatabaseValue()
         $this->assertEquals('foo', $actual);
     }
 
-    public function testNotSupportedUlidStringConversionToDatabaseValue()
+    public function testUlidStringConvertsToDatabaseValue()
     {
-        $this->expectException(ConversionException::class);
+        $actual = $this->type->convertToDatabaseValue(self::DUMMY_ULID, $this->platform);
+        $ulid = Ulid::fromString(self::DUMMY_ULID);
 
-        $this->type->convertToDatabaseValue(self::DUMMY_ULID, $this->platform);
+        $expected = $ulid->toRfc4122();
+
+        $this->assertEquals($expected, $actual);
     }
 
     public function testNotSupportedTypeConversionForDatabaseValue()
diff --git a/src/Symfony/Bridge/Doctrine/Tests/Types/UuidBinaryTypeTest.php b/src/Symfony/Bridge/Doctrine/Tests/Types/UuidBinaryTypeTest.php
@@ -52,11 +52,21 @@ public function testUuidConvertsToDatabaseValue()
         $this->assertEquals($expected, $actual);
     }
 
-    public function testNotSupportedStringUuidConversionToDatabaseValue()
+    public function testStringUuidConvertsToDatabaseValue()
+    {
+        $uuid = self::DUMMY_UUID;
+
+        $expected = uuid_parse(self::DUMMY_UUID);
+        $actual = $this->type->convertToDatabaseValue($uuid, $this->platform);
+
+        $this->assertEquals($expected, $actual);
+    }
+
+    public function testInvalidUuidConversionForDatabaseValue()
     {
         $this->expectException(ConversionException::class);
 
-        $this->type->convertToDatabaseValue(self::DUMMY_UUID, $this->platform);
+        $this->type->convertToDatabaseValue('abcdefg', $this->platform);
     }
 
     public function testNullConversionForDatabaseValue()
diff --git a/src/Symfony/Bridge/Doctrine/Tests/Types/UuidTypeTest.php b/src/Symfony/Bridge/Doctrine/Tests/Types/UuidTypeTest.php
@@ -68,11 +68,11 @@ public function testUuidInterfaceConvertsToDatabaseValue()
         $this->assertEquals('foo', $actual);
     }
 
-    public function testNotSupportedUuidStringConversionToDatabaseValue()
+    public function testUuidStringConvertsToDatabaseValue()
     {
-        $this->expectException(ConversionException::class);
+        $actual = $this->type->convertToDatabaseValue(self::DUMMY_UUID, $this->platform);
 
-        $this->type->convertToDatabaseValue(self::DUMMY_UUID, $this->platform);
+        $this->assertEquals(self::DUMMY_UUID, $actual);
     }
 
     public function testNotSupportedTypeConversionForDatabaseValue()
diff --git a/src/Symfony/Bridge/Doctrine/Types/AbstractBinaryUidType.php b/src/Symfony/Bridge/Doctrine/Types/AbstractBinaryUidType.php
@@ -61,11 +61,19 @@ public function convertToDatabaseValue($value, AbstractPlatform $platform): ?str
             return $value->toBinary();
         }
 
-        if (null === $value) {
+        if (null === $value || '' === $value) {
             return null;
         }
 
-        throw ConversionException::conversionFailedInvalidType($value, $this->getName(), ['null', AbstractUid::class]);
+        if (!\is_string($value)) {
+            throw ConversionException::conversionFailedInvalidType($value, $this->getName(), ['null', 'string', AbstractUid::class]);
+        }
+
+        try {
+            return $this->getUidClass()::fromString($value)->toBinary();
+        } catch (\InvalidArgumentException $e) {
+            throw ConversionException::conversionFailed($value, $this->getName());
+        }
     }
 
     /**
diff --git a/src/Symfony/Bridge/Doctrine/Types/AbstractUidType.php b/src/Symfony/Bridge/Doctrine/Types/AbstractUidType.php
@@ -61,11 +61,19 @@ public function convertToDatabaseValue($value, AbstractPlatform $platform): ?str
             return $value->toRfc4122();
         }
 
-        if (null === $value) {
+        if (null === $value || '' === $value) {
             return null;
         }
 
-        throw ConversionException::conversionFailedInvalidType($value, $this->getName(), ['null', AbstractUid::class]);
+        if (!\is_string($value)) {
+            throw ConversionException::conversionFailedInvalidType($value, $this->getName(), ['null', 'string', AbstractUid::class]);
+        }
+
+        try {
+            return $this->getUidClass()::fromString($value)->toRfc4122();
+        } catch (\InvalidArgumentException $e) {
+            throw ConversionException::conversionFailed($value, $this->getName());
+        }
     }
 
     /**

Original file line number	Diff line number	Diff line change
`@@ -52,11 +52,12 @@ public function testUlidConvertsToDatabaseValue()`
`52`	`52`	`$this->assertEquals($expected, $actual);`
`53`	`53`	`}`
`54`	`54`
`55`		`- public function testNotSupportedStringUlidConversionToDatabaseValue()`
	`55`	`+ public function testStringUlidConvertsToDatabaseValue()`
`56`	`56`	`{`
`57`		`- $this->expectException(ConversionException::class);`
	`57`	`+ $expected = Ulid::fromString(self::DUMMY_ULID)->toBinary();`
	`58`	`+ $actual = $this->type->convertToDatabaseValue(self::DUMMY_ULID, $this->platform);`
`58`	`59`
`59`		`- $this->type->convertToDatabaseValue(self::DUMMY_ULID, $this->platform);`
	`60`	`+ $this->assertEquals($expected, $actual);`
`60`	`61`	`}`
`61`	`62`
`62`	`63`	`public function testNotSupportedTypeConversionForDatabaseValue()`
Original file line number	Diff line number	Diff line change
`@@ -68,11 +68,14 @@ public function testUlidInterfaceConvertsToDatabaseValue()`
`68`	`68`	`$this->assertEquals('foo', $actual);`
`69`	`69`	`}`
`70`	`70`
`71`		`- public function testNotSupportedUlidStringConversionToDatabaseValue()`
	`71`	`+ public function testUlidStringConvertsToDatabaseValue()`
`72`	`72`	`{`
`73`		`- $this->expectException(ConversionException::class);`
	`73`	`+ $actual = $this->type->convertToDatabaseValue(self::DUMMY_ULID, $this->platform);`
	`74`	`+ $ulid = Ulid::fromString(self::DUMMY_ULID);`
`74`	`75`
`75`		`- $this->type->convertToDatabaseValue(self::DUMMY_ULID, $this->platform);`
	`76`	`+ $expected = $ulid->toRfc4122();`
	`77`	`+`
	`78`	`+ $this->assertEquals($expected, $actual);`
`76`	`79`	`}`
`77`	`80`
`78`	`81`	`public function testNotSupportedTypeConversionForDatabaseValue()`
Original file line number	Diff line number	Diff line change
`@@ -68,11 +68,11 @@ public function testUuidInterfaceConvertsToDatabaseValue()`
`68`	`68`	`$this->assertEquals('foo', $actual);`
`69`	`69`	`}`
`70`	`70`
`71`		`- public function testNotSupportedUuidStringConversionToDatabaseValue()`
	`71`	`+ public function testUuidStringConvertsToDatabaseValue()`
`72`	`72`	`{`
`73`		`- $this->expectException(ConversionException::class);`
	`73`	`+ $actual = $this->type->convertToDatabaseValue(self::DUMMY_UUID, $this->platform);`
`74`	`74`
`75`		`- $this->type->convertToDatabaseValue(self::DUMMY_UUID, $this->platform);`
	`75`	`+ $this->assertEquals(self::DUMMY_UUID, $actual);`
`76`	`76`	`}`
`77`	`77`
`78`	`78`	`public function testNotSupportedTypeConversionForDatabaseValue()`
Original file line number	Diff line number	Diff line change
`@@ -61,11 +61,19 @@ public function convertToDatabaseValue($value, AbstractPlatform $platform): ?str`
`61`	`61`	`return $value->toBinary();`
`62`	`62`	`}`
`63`	`63`
`64`		`- if (null === $value) {`
	`64`	`+ if (null === $value \|\| '' === $value) {`
`65`	`65`	`return null;`
`66`	`66`	`}`
`67`	`67`
`68`		`- throw ConversionException::conversionFailedInvalidType($value, $this->getName(), ['null', AbstractUid::class]);`
	`68`	`+ if (!\is_string($value)) {`
	`69`	`+ throw ConversionException::conversionFailedInvalidType($value, $this->getName(), ['null', 'string', AbstractUid::class]);`
	`70`	`+ }`
	`71`	`+`
	`72`	`+ try {`
	`73`	`+ return $this->getUidClass()::fromString($value)->toBinary();`
	`74`	`+ } catch (\InvalidArgumentException $e) {`
	`75`	`+ throw ConversionException::conversionFailed($value, $this->getName());`
	`76`	`+ }`
`69`	`77`	`}`
`70`	`78`
`71`	`79`	`/**`