From fb4476cd32ed97b65b9823019b6d3c156e05244f Mon Sep 17 00:00:00 2001 From: Sergei Morozov Date: Wed, 6 Jun 2018 14:38:36 -0700 Subject: [PATCH] Removed the logic of using BLOB columns for BINARY type of fields --- UPGRADE.md | 6 ++++ .../DBAL/Platforms/AbstractPlatform.php | 16 --------- lib/Doctrine/DBAL/Types/BinaryType.php | 15 +++----- .../DBAL/Functional/Types/BinaryTest.php | 13 ++----- .../AbstractMySQLPlatformTestCase.php | 32 +++++++---------- .../Platforms/AbstractPlatformTestCase.php | 5 --- .../AbstractSQLServerPlatformTestCase.php | 20 +++++------ .../Tests/DBAL/Platforms/DB2PlatformTest.php | 10 ------ .../DBAL/Platforms/OraclePlatformTest.php | 20 +++++------ .../Platforms/SQLAnywherePlatformTest.php | 20 +++++------ .../Doctrine/Tests/DBAL/Types/BinaryTest.php | 19 +++++++--- tests/Doctrine/Tests/DBAL/Types/BlobTest.php | 35 ------------------- 12 files changed, 63 insertions(+), 148 deletions(-) diff --git a/UPGRADE.md b/UPGRADE.md index adb215a3e86..6d2e3922d30 100644 --- a/UPGRADE.md +++ b/UPGRADE.md @@ -102,6 +102,12 @@ The Drizzle project is abandoned and is therefore not supported by Doctrine DBAL - `Configuration::getSQLLogger()` does not return `null` anymore, but a `NullLogger` implementation. - `Configuration::setSQLLogger()` does not allow `null` anymore. +## BC BREAK: Changes to handling binary fields + +- Binary fields whose length exceeds the maximum field size on a given platform are no longer represented as `BLOB`s. + Use binary fields of a size which fits all target platforms, or use blob explicitly instead. +- Binary fields are no longer represented as streams in PHP. They are represented as strings. + # Upgrade to 2.10 ## Deprecated `Doctrine\DBAL\Driver::getName()` diff --git a/lib/Doctrine/DBAL/Platforms/AbstractPlatform.php b/lib/Doctrine/DBAL/Platforms/AbstractPlatform.php index 1e7548596fd..80f78f37cd5 100644 --- a/lib/Doctrine/DBAL/Platforms/AbstractPlatform.php +++ b/lib/Doctrine/DBAL/Platforms/AbstractPlatform.php @@ -29,7 +29,6 @@ use Doctrine\DBAL\Types\Type; use InvalidArgumentException; use UnexpectedValueException; -use const E_USER_DEPRECATED; use function addcslashes; use function array_map; use function array_merge; @@ -55,7 +54,6 @@ use function strpos; use function strtolower; use function strtoupper; -use function trigger_error; /** * Base class for all DatabasePlatforms. The DatabasePlatforms are the central @@ -283,20 +281,6 @@ public function getBinaryTypeDeclarationSQL(array $field) $fixed = $field['fixed'] ?? false; - $maxLength = $this->getBinaryMaxLength(); - - if ($field['length'] > $maxLength) { - if ($maxLength > 0) { - @trigger_error(sprintf( - 'Binary field length %d is greater than supported by the platform (%d). Reduce the field length or use a BLOB field instead.', - $field['length'], - $maxLength - ), E_USER_DEPRECATED); - } - - return $this->getBlobTypeDeclarationSQL($field); - } - return $this->getBinaryTypeDeclarationSQLSnippet($field['length'], $fixed); } diff --git a/lib/Doctrine/DBAL/Types/BinaryType.php b/lib/Doctrine/DBAL/Types/BinaryType.php index d604b3bff69..8aefd610401 100644 --- a/lib/Doctrine/DBAL/Types/BinaryType.php +++ b/lib/Doctrine/DBAL/Types/BinaryType.php @@ -4,12 +4,9 @@ use Doctrine\DBAL\ParameterType; use Doctrine\DBAL\Platforms\AbstractPlatform; -use function assert; -use function fopen; -use function fseek; -use function fwrite; use function is_resource; use function is_string; +use function stream_get_contents; /** * Type that maps ab SQL BINARY/VARBINARY to a PHP resource stream. @@ -33,15 +30,11 @@ public function convertToPHPValue($value, AbstractPlatform $platform) return null; } - if (is_string($value)) { - $fp = fopen('php://temp', 'rb+'); - assert(is_resource($fp)); - fwrite($fp, $value); - fseek($fp, 0); - $value = $fp; + if (is_resource($value)) { + $value = stream_get_contents($value); } - if (! is_resource($value)) { + if (! is_string($value)) { throw ConversionException::conversionFailed($value, Types::BINARY); } diff --git a/tests/Doctrine/Tests/DBAL/Functional/Types/BinaryTest.php b/tests/Doctrine/Tests/DBAL/Functional/Types/BinaryTest.php index 76236acff0c..676ad97cecf 100644 --- a/tests/Doctrine/Tests/DBAL/Functional/Types/BinaryTest.php +++ b/tests/Doctrine/Tests/DBAL/Functional/Types/BinaryTest.php @@ -8,11 +8,10 @@ use Doctrine\DBAL\Driver\PDOOracle\Driver as PDOOracleDriver; use Doctrine\DBAL\ParameterType; use Doctrine\DBAL\Schema\Table; +use Doctrine\DBAL\Types\Type; use Doctrine\Tests\DbalFunctionalTestCase; -use function is_resource; use function random_bytes; use function str_replace; -use function stream_get_contents; class BinaryTest extends DbalFunctionalTestCase { @@ -82,14 +81,6 @@ private function select(string $id) [ParameterType::BINARY] ); - // Currently, `BinaryType` mistakenly converts string values fetched from the DB to a stream. - // It should be the opposite. Streams should be used to represent large objects, not binary - // strings. The confusion comes from the PostgreSQL's type system where binary strings and - // large objects are represented by the same BYTEA type - if (is_resource($value)) { - $value = stream_get_contents($value); - } - - return $value; + return Type::getType('binary')->convertToPHPValue($value, $this->connection->getDatabasePlatform()); } } diff --git a/tests/Doctrine/Tests/DBAL/Platforms/AbstractMySQLPlatformTestCase.php b/tests/Doctrine/Tests/DBAL/Platforms/AbstractMySQLPlatformTestCase.php index bc71767a766..2d59826272d 100644 --- a/tests/Doctrine/Tests/DBAL/Platforms/AbstractMySQLPlatformTestCase.php +++ b/tests/Doctrine/Tests/DBAL/Platforms/AbstractMySQLPlatformTestCase.php @@ -538,27 +538,21 @@ public function testReturnsBinaryTypeDeclarationSQL() : void self::assertSame('VARBINARY(255)', $this->platform->getBinaryTypeDeclarationSQL([])); self::assertSame('VARBINARY(255)', $this->platform->getBinaryTypeDeclarationSQL(['length' => 0])); self::assertSame('VARBINARY(65535)', $this->platform->getBinaryTypeDeclarationSQL(['length' => 65535])); + self::assertSame('VARBINARY(65536)', $this->platform->getBinaryTypeDeclarationSQL(['length' => 65536])); self::assertSame('BINARY(255)', $this->platform->getBinaryTypeDeclarationSQL(['fixed' => true])); - self::assertSame('BINARY(255)', $this->platform->getBinaryTypeDeclarationSQL(['fixed' => true, 'length' => 0])); - self::assertSame('BINARY(65535)', $this->platform->getBinaryTypeDeclarationSQL(['fixed' => true, 'length' => 65535])); - } - - /** - * @group legacy - * @expectedDeprecation Binary field length 65536 is greater than supported by the platform (65535). Reduce the field length or use a BLOB field instead. - * @expectedDeprecation Binary field length 16777215 is greater than supported by the platform (65535). Reduce the field length or use a BLOB field instead. - * @expectedDeprecation Binary field length 16777216 is greater than supported by the platform (65535). Reduce the field length or use a BLOB field instead. - */ - public function testReturnsBinaryTypeLongerThanMaxDeclarationSQL() : void - { - self::assertSame('MEDIUMBLOB', $this->platform->getBinaryTypeDeclarationSQL(['length' => 65536])); - self::assertSame('MEDIUMBLOB', $this->platform->getBinaryTypeDeclarationSQL(['length' => 16777215])); - self::assertSame('LONGBLOB', $this->platform->getBinaryTypeDeclarationSQL(['length' => 16777216])); - - self::assertSame('MEDIUMBLOB', $this->platform->getBinaryTypeDeclarationSQL(['fixed' => true, 'length' => 65536])); - self::assertSame('MEDIUMBLOB', $this->platform->getBinaryTypeDeclarationSQL(['fixed' => true, 'length' => 16777215])); - self::assertSame('LONGBLOB', $this->platform->getBinaryTypeDeclarationSQL(['fixed' => true, 'length' => 16777216])); + self::assertSame('BINARY(255)', $this->platform->getBinaryTypeDeclarationSQL([ + 'fixed' => true, + 'length' => 0, + ])); + self::assertSame('BINARY(65535)', $this->platform->getBinaryTypeDeclarationSQL([ + 'fixed' => true, + 'length' => 65535, + ])); + self::assertSame('BINARY(65536)', $this->platform->getBinaryTypeDeclarationSQL([ + 'fixed' => true, + 'length' => 65536, + ])); } public function testDoesNotPropagateForeignKeyCreationForNonSupportingEngines() : void diff --git a/tests/Doctrine/Tests/DBAL/Platforms/AbstractPlatformTestCase.php b/tests/Doctrine/Tests/DBAL/Platforms/AbstractPlatformTestCase.php index 3584de8ae3d..3ce6e1132c4 100644 --- a/tests/Doctrine/Tests/DBAL/Platforms/AbstractPlatformTestCase.php +++ b/tests/Doctrine/Tests/DBAL/Platforms/AbstractPlatformTestCase.php @@ -862,11 +862,6 @@ public function testReturnsBinaryTypeDeclarationSQL() : void $this->platform->getBinaryTypeDeclarationSQL([]); } - public function testReturnsBinaryTypeLongerThanMaxDeclarationSQL() : void - { - $this->markTestSkipped('Not applicable to the platform'); - } - /** * @group DBAL-553 */ diff --git a/tests/Doctrine/Tests/DBAL/Platforms/AbstractSQLServerPlatformTestCase.php b/tests/Doctrine/Tests/DBAL/Platforms/AbstractSQLServerPlatformTestCase.php index edbc870462a..fb9e5765af2 100644 --- a/tests/Doctrine/Tests/DBAL/Platforms/AbstractSQLServerPlatformTestCase.php +++ b/tests/Doctrine/Tests/DBAL/Platforms/AbstractSQLServerPlatformTestCase.php @@ -1008,18 +1008,14 @@ public function testReturnsBinaryTypeDeclarationSQL() : void self::assertSame('VARBINARY(8000)', $this->platform->getBinaryTypeDeclarationSQL(['length' => 8000])); self::assertSame('BINARY(255)', $this->platform->getBinaryTypeDeclarationSQL(['fixed' => true])); - self::assertSame('BINARY(255)', $this->platform->getBinaryTypeDeclarationSQL(['fixed' => true, 'length' => 0])); - self::assertSame('BINARY(8000)', $this->platform->getBinaryTypeDeclarationSQL(['fixed' => true, 'length' => 8000])); - } - - /** - * @group legacy - * @expectedDeprecation Binary field length 8001 is greater than supported by the platform (8000). Reduce the field length or use a BLOB field instead. - */ - public function testReturnsBinaryTypeLongerThanMaxDeclarationSQL() : void - { - self::assertSame('VARBINARY(MAX)', $this->platform->getBinaryTypeDeclarationSQL(['length' => 8001])); - self::assertSame('VARBINARY(MAX)', $this->platform->getBinaryTypeDeclarationSQL(['fixed' => true, 'length' => 8001])); + self::assertSame('BINARY(255)', $this->platform->getBinaryTypeDeclarationSQL([ + 'fixed' => true, + 'length' => 0, + ])); + self::assertSame('BINARY(8000)', $this->platform->getBinaryTypeDeclarationSQL([ + 'fixed' => true, + 'length' => 8000, + ])); } /** diff --git a/tests/Doctrine/Tests/DBAL/Platforms/DB2PlatformTest.php b/tests/Doctrine/Tests/DBAL/Platforms/DB2PlatformTest.php index a0c1f1f4b4b..e2495e0f2f6 100644 --- a/tests/Doctrine/Tests/DBAL/Platforms/DB2PlatformTest.php +++ b/tests/Doctrine/Tests/DBAL/Platforms/DB2PlatformTest.php @@ -446,16 +446,6 @@ public function testReturnsBinaryTypeDeclarationSQL() : void self::assertSame('CHAR(254) FOR BIT DATA', $this->platform->getBinaryTypeDeclarationSQL(['fixed' => true, 'length' => 0])); } - /** - * @group legacy - * @expectedDeprecation Binary field length 32705 is greater than supported by the platform (32704). Reduce the field length or use a BLOB field instead. - */ - public function testReturnsBinaryTypeLongerThanMaxDeclarationSQL() : void - { - self::assertSame('BLOB(1M)', $this->platform->getBinaryTypeDeclarationSQL(['length' => 32705])); - self::assertSame('BLOB(1M)', $this->platform->getBinaryTypeDeclarationSQL(['fixed' => true, 'length' => 32705])); - } - /** * {@inheritDoc} * diff --git a/tests/Doctrine/Tests/DBAL/Platforms/OraclePlatformTest.php b/tests/Doctrine/Tests/DBAL/Platforms/OraclePlatformTest.php index 0dd4b304739..68d7c749a44 100644 --- a/tests/Doctrine/Tests/DBAL/Platforms/OraclePlatformTest.php +++ b/tests/Doctrine/Tests/DBAL/Platforms/OraclePlatformTest.php @@ -513,18 +513,14 @@ public function testReturnsBinaryTypeDeclarationSQL() : void self::assertSame('RAW(2000)', $this->platform->getBinaryTypeDeclarationSQL(['length' => 2000])); self::assertSame('RAW(255)', $this->platform->getBinaryTypeDeclarationSQL(['fixed' => true])); - self::assertSame('RAW(2000)', $this->platform->getBinaryTypeDeclarationSQL(['fixed' => true, 'length' => 0])); - self::assertSame('RAW(2000)', $this->platform->getBinaryTypeDeclarationSQL(['fixed' => true, 'length' => 2000])); - } - - /** - * @group legacy - * @expectedDeprecation Binary field length 2001 is greater than supported by the platform (2000). Reduce the field length or use a BLOB field instead. - */ - public function testReturnsBinaryTypeLongerThanMaxDeclarationSQL() : void - { - self::assertSame('BLOB', $this->platform->getBinaryTypeDeclarationSQL(['length' => 2001])); - self::assertSame('BLOB', $this->platform->getBinaryTypeDeclarationSQL(['fixed' => true, 'length' => 2001])); + self::assertSame('RAW(2000)', $this->platform->getBinaryTypeDeclarationSQL([ + 'fixed' => true, + 'length' => 0, + ])); + self::assertSame('RAW(2000)', $this->platform->getBinaryTypeDeclarationSQL([ + 'fixed' => true, + 'length' => 2000, + ])); } public function testDoesNotPropagateUnnecessaryTableAlterationOnBinaryType() : void diff --git a/tests/Doctrine/Tests/DBAL/Platforms/SQLAnywherePlatformTest.php b/tests/Doctrine/Tests/DBAL/Platforms/SQLAnywherePlatformTest.php index cdf5552f551..b9d9655063e 100644 --- a/tests/Doctrine/Tests/DBAL/Platforms/SQLAnywherePlatformTest.php +++ b/tests/Doctrine/Tests/DBAL/Platforms/SQLAnywherePlatformTest.php @@ -966,18 +966,14 @@ public function testReturnsBinaryTypeDeclarationSQL() : void self::assertSame('VARBINARY(32767)', $this->platform->getBinaryTypeDeclarationSQL(['length' => 32767])); self::assertSame('BINARY(1)', $this->platform->getBinaryTypeDeclarationSQL(['fixed' => true])); - self::assertSame('BINARY(1)', $this->platform->getBinaryTypeDeclarationSQL(['fixed' => true, 'length' => 0])); - self::assertSame('BINARY(32767)', $this->platform->getBinaryTypeDeclarationSQL(['fixed' => true, 'length' => 32767])); - } - - /** - * @group legacy - * @expectedDeprecation Binary field length 32768 is greater than supported by the platform (32767). Reduce the field length or use a BLOB field instead. - */ - public function testReturnsBinaryTypeLongerThanMaxDeclarationSQL() : void - { - self::assertSame('LONG BINARY', $this->platform->getBinaryTypeDeclarationSQL(['length' => 32768])); - self::assertSame('LONG BINARY', $this->platform->getBinaryTypeDeclarationSQL(['fixed' => true, 'length' => 32768])); + self::assertSame('BINARY(1)', $this->platform->getBinaryTypeDeclarationSQL([ + 'fixed' => true, + 'length' => 0, + ])); + self::assertSame('BINARY(32767)', $this->platform->getBinaryTypeDeclarationSQL([ + 'fixed' => true, + 'length' => 32767, + ])); } /** diff --git a/tests/Doctrine/Tests/DBAL/Types/BinaryTest.php b/tests/Doctrine/Tests/DBAL/Types/BinaryTest.php index fceeee557b8..259e3daa053 100644 --- a/tests/Doctrine/Tests/DBAL/Types/BinaryTest.php +++ b/tests/Doctrine/Tests/DBAL/Types/BinaryTest.php @@ -10,9 +10,11 @@ use Doctrine\DBAL\Types\Types; use Doctrine\Tests\DbalTestCase; use PHPUnit\Framework\MockObject\MockObject; +use function array_map; use function base64_encode; use function fopen; -use function stream_get_contents; +use function implode; +use function range; class BinaryTest extends DbalTestCase { @@ -57,11 +59,10 @@ public function testBinaryNullConvertsToPHPValue() : void public function testBinaryStringConvertsToPHPValue() : void { - $databaseValue = 'binary string'; + $databaseValue = $this->getBinaryString(); $phpValue = $this->type->convertToPHPValue($databaseValue, $this->platform); - self::assertIsResource($phpValue); - self::assertEquals($databaseValue, stream_get_contents($phpValue)); + self::assertSame($databaseValue, $phpValue); } public function testBinaryResourceConvertsToPHPValue() : void @@ -69,7 +70,15 @@ public function testBinaryResourceConvertsToPHPValue() : void $databaseValue = fopen('data://text/plain;base64,' . base64_encode('binary string'), 'r'); $phpValue = $this->type->convertToPHPValue($databaseValue, $this->platform); - self::assertSame($databaseValue, $phpValue); + self::assertSame('binary string', $phpValue); + } + + /** + * Creates a binary string containing all possible byte values. + */ + private function getBinaryString() : string + { + return implode(array_map('chr', range(0, 255))); } /** diff --git a/tests/Doctrine/Tests/DBAL/Types/BlobTest.php b/tests/Doctrine/Tests/DBAL/Types/BlobTest.php index 194d26af0b8..40fb111f1ba 100644 --- a/tests/Doctrine/Tests/DBAL/Types/BlobTest.php +++ b/tests/Doctrine/Tests/DBAL/Types/BlobTest.php @@ -7,10 +7,6 @@ use Doctrine\DBAL\Types\Type; use Doctrine\Tests\DbalTestCase; use PHPUnit\Framework\MockObject\MockObject; -use function base64_encode; -use function chr; -use function fopen; -use function stream_get_contents; class BlobTest extends DbalTestCase { @@ -33,35 +29,4 @@ public function testBlobNullConvertsToPHPValue() : void { self::assertNull($this->type->convertToPHPValue(null, $this->platform)); } - - public function testBinaryStringConvertsToPHPValue() : void - { - $databaseValue = $this->getBinaryString(); - $phpValue = $this->type->convertToPHPValue($databaseValue, $this->platform); - - self::assertIsResource($phpValue); - self::assertSame($databaseValue, stream_get_contents($phpValue)); - } - - public function testBinaryResourceConvertsToPHPValue() : void - { - $databaseValue = fopen('data://text/plain;base64,' . base64_encode($this->getBinaryString()), 'r'); - $phpValue = $this->type->convertToPHPValue($databaseValue, $this->platform); - - self::assertSame($databaseValue, $phpValue); - } - - /** - * Creates a binary string containing all possible byte values. - */ - private function getBinaryString() : string - { - $string = ''; - - for ($i = 0; $i < 256; $i++) { - $string .= chr($i); - } - - return $string; - } }