Skip to content

Commit

Permalink
Merge pull request #947 from alcaeus/deprecate-type-commenting
Browse files Browse the repository at this point in the history 
Deprecate type comment attribute from configuration
  • Loading branch information
@alcaeus
alcaeus committed Apr 12, 2019
2 parents f5c10ec + ba3f5bb commit 5c79bbc
Show file tree
Hide file tree
Showing 7 changed files with 233 additions and 21 deletions.
92 changes: 77 additions & 15 deletions ConnectionFactory.php
View file
Expand Up @@ -10,15 +10,15 @@
use Doctrine\DBAL\Exception\DriverException;
use Doctrine\DBAL\Platforms\AbstractPlatform;
use Doctrine\DBAL\Types\Type;
use const E_USER_DEPRECATED;
use function get_class;
use function trigger_error;

class ConnectionFactory
{
/** @var mixed[][] */
private $typesConfig = [];

/** @var string[] */
private $commentedTypes = [];

/** @var bool */
private $initialized = false;

Expand Down Expand Up @@ -52,11 +52,9 @@ public function createConnection(array $params, Configuration $config = null, Ev
$platform->registerDoctrineTypeMapping($dbType, $doctrineType);
}
}
if (! empty($this->commentedTypes)) {
$platform = $this->getDatabasePlatform($connection);
foreach ($this->commentedTypes as $type) {
$platform->markDoctrineTypeCommented(Type::getType($type));
}

if (! empty($this->typesConfig)) {
$this->markTypesCommented($this->getDatabasePlatform($connection));
}

return $connection;
Expand Down Expand Up @@ -94,18 +92,82 @@ private function getDatabasePlatform(Connection $connection)
*/
private function initializeTypes()
{
foreach ($this->typesConfig as $type => $typeConfig) {
if (Type::hasType($type)) {
Type::overrideType($type, $typeConfig['class']);
foreach ($this->typesConfig as $typeName => $typeConfig) {
if (Type::hasType($typeName)) {
Type::overrideType($typeName, $typeConfig['class']);
} else {
Type::addType($type, $typeConfig['class']);
Type::addType($typeName, $typeConfig['class']);
}
if (! $typeConfig['commented']) {
}

$this->initialized = true;
}

private function markTypesCommented(AbstractPlatform $platform) : void
{
foreach ($this->typesConfig as $typeName => $typeConfig) {
$type = Type::getType($typeName);
$requiresSQLCommentHint = $type->requiresSQLCommentHint($platform);

// Attribute is missing, make sure a type that doesn't require a comment is marked as commented
// This is deprecated behaviour that will be dropped in 2.0.
if ($typeConfig['commented'] === null) {
if (! $requiresSQLCommentHint) {
@trigger_error(
sprintf(
'The type "%s" was implicitly marked as commented due to the configuration. This is deprecated and will be removed in DoctrineBundle 2.0. Either set the "commented" attribute in the configuration to "false" or mark the type as commented in "%s::requiresSQLCommentHint()."',
$typeName,
get_class($type)
),
E_USER_DEPRECATED
);

$platform->markDoctrineTypeCommented($type);
}

continue;
}

$this->commentedTypes[] = $type;
// The following logic generates appropriate deprecation notices telling the user how to update their type configuration.
if ($typeConfig['commented']) {
if (! $requiresSQLCommentHint) {
@trigger_error(
sprintf(
'The type "%s" was marked as commented in its configuration but not in the type itself. This is deprecated and will be removed in DoctrineBundle 2.0. Please update the return value of "%s::requiresSQLCommentHint()."',
$typeName,
get_class($type)
),
E_USER_DEPRECATED
);

$platform->markDoctrineTypeCommented($type);

continue;
}

@trigger_error(
sprintf(
'The type "%s" was explicitly marked as commented in its configuration. This is no longer necessary and will be removed in DoctrineBundle 2.0. Please remove the "commented" attribute from the type configuration.',
$typeName
),
E_USER_DEPRECATED
);

continue;
}

if (! $requiresSQLCommentHint) {
continue;
}

@trigger_error(
sprintf(
'The type "%s" was marked as uncommented in its configuration but commented in the type itself. This is deprecated and will be removed in DoctrineBundle 2.0. Please update the return value of "%s::requiresSQLCommentHint()" or remove the "commented" attribute from the type configuration.',
$typeName,
get_class($type)
),
E_USER_DEPRECATED
);
}
$this->initialized = true;
}
}
2 changes: 1 addition & 1 deletion DependencyInjection/Configuration.php
View file
Expand Up @@ -96,7 +96,7 @@ private function addDbalSection(ArrayNodeDefinition $node)
->end()
->children()
->scalarNode('class')->isRequired()->end()
->booleanNode('commented')->defaultTrue()->end()
->booleanNode('commented')->defaultNull()->end()
->end()
->end()
->end()
Expand Down
125 changes: 122 additions & 3 deletions Tests/ConnectionFactoryTest.php
View file
Expand Up @@ -3,9 +3,13 @@
namespace Doctrine\Bundle\DoctrineBundle\Tests;

use Doctrine\Bundle\DoctrineBundle\ConnectionFactory;
use Doctrine\Bundle\DoctrineBundle\Tests\DependencyInjection\TestCommentedType;
use Doctrine\Bundle\DoctrineBundle\Tests\DependencyInjection\TestType;
use Doctrine\DBAL\Connection;
use Doctrine\DBAL\Driver;
use Doctrine\DBAL\Exception\DriverException;
use Doctrine\DBAL\Platforms\AbstractPlatform;
use Doctrine\DBAL\Platforms\MySqlPlatform;
use Doctrine\ORM\Version;
use Exception;

Expand All @@ -29,11 +33,11 @@ public function testContainer()
{
$typesConfig = [];
$factory = new ConnectionFactory($typesConfig);
$params = ['driverClass' => '\\Doctrine\\Bundle\\DoctrineBundle\\Tests\\FakeDriver'];
$params = ['driverClass' => FakeDriver::class];
$config = null;
$eventManager = null;
$mappingTypes = [0];
$exception = new DriverException('', $this->getMockBuilder(Driver\AbstractDriverException::class)->disableOriginalConstructor()->getMock());
$exception = new DriverException('', $this->createMock(Driver\AbstractDriverException::class));

// put the mock into the fake driver
FakeDriver::$exception = $exception;
Expand All @@ -43,8 +47,116 @@ public function testContainer()
} catch (Exception $e) {
$this->assertTrue(strpos($e->getMessage(), 'can circumvent this by setting') > 0);
throw $e;
} finally {
FakeDriver::$exception = null;
}
}

/**
* @dataProvider getValidTypeConfigurations
*/
public function testRegisterTypes(array $type, int $expectedCalls) : void
{
$factory = new ConnectionFactory(['test' => $type]);
$params = ['driverClass' => FakeDriver::class];
$config = null;
$eventManager = null;
$mappingTypes = [];

$platform = $this->createMock(AbstractPlatform::class);
$platform
->expects($this->exactly($expectedCalls))
->method('markDoctrineTypeCommented')
->with($this->isInstanceOf($type['class']));

FakeDriver::$platform = $platform;

try {
$factory->createConnection($params, $config, $eventManager, $mappingTypes);
} finally {
FakeDriver::$platform = null;
}
}

public static function getValidTypeConfigurations() : array
{
return [
'uncommentedTypeMarkedNotCommented' => [
'type' => [
'class' => TestType::class,
'commented' => false,
],
'expectedCalls' => 0,
],
'commentedTypeNotMarked' => [
'type' => [
'class' => TestCommentedType::class,
'commented' => null,
],
'expectedCalls' => 0,
],
];
}

/**
* @group legacy
* @expectedDeprecation The type "test" was implicitly marked as commented due to the configuration. This is deprecated and will be removed in DoctrineBundle 2.0. Either set the "commented" attribute in the configuration to "false" or mark the type as commented in "Doctrine\Bundle\DoctrineBundle\Tests\DependencyInjection\TestType::requiresSQLCommentHint()."
*/
public function testRegisterUncommentedTypeNotMarked() : void
{
$this->testRegisterTypes(
[
'class' => TestType::class,
'commented' => null,
],
1
);
}

/**
* @group legacy
* @expectedDeprecation The type "test" was marked as commented in its configuration but not in the type itself. This is deprecated and will be removed in DoctrineBundle 2.0. Please update the return value of "Doctrine\Bundle\DoctrineBundle\Tests\DependencyInjection\TestType::requiresSQLCommentHint()."
*/
public function testRegisterUncommentedTypeMarkedCommented() : void
{
$this->testRegisterTypes(
[
'class' => TestType::class,
'commented' => true,
],
1
);
}

/**
* @group legacy
* @expectedDeprecation The type "test" was explicitly marked as commented in its configuration. This is no longer necessary and will be removed in DoctrineBundle 2.0. Please remove the "commented" attribute from the type configuration.
*/
public function testRegisterCommentedTypeMarkedCommented() : void
{
$this->testRegisterTypes(
[
'class' => TestCommentedType::class,
'commented' => true,
],
0
);
}

/**
* @group legacy
* @expectedDeprecation The type "test" was marked as uncommented in its configuration but commented in the type itself. This is deprecated and will be removed in DoctrineBundle 2.0. Please update the return value of "Doctrine\Bundle\DoctrineBundle\Tests\DependencyInjection\TestCommentedType::requiresSQLCommentHint()" or remove the "commented" attribute from the type configuration.
*/
public function testRegisterCommentedTypeMarkedNotCommented() : void
{
$this->testRegisterTypes(
[
'class' => TestCommentedType::class,
'commented' => false,
],
0
);
}
}

/**
Expand All @@ -62,6 +174,9 @@ class FakeDriver implements Driver
*/
public static $exception;

/** @var AbstractPlatform|null */
public static $platform;

/**
* This method gets called to determine the database version which in our case leeds to the problem.
* So we have to fake the exception a driver would normally throw.
Expand All @@ -70,7 +185,11 @@ class FakeDriver implements Driver
*/
public function getDatabasePlatform()
{
throw self::$exception;
if (self::$exception !== null) {
throw self::$exception;
}

return static::$platform ?? new MySqlPlatform();
}

// ----- below this line follow only dummy methods to satisfy the interface requirements ----
Expand Down
2 changes: 1 addition & 1 deletion Tests/DependencyInjection/AbstractDoctrineExtensionTest.php
View file
Expand Up @@ -526,7 +526,7 @@ public function testSetTypes()
$container = $this->loadContainer('dbal_types');

$this->assertEquals(
['test' => ['class' => TestType::class, 'commented' => true]],
['test' => ['class' => TestType::class, 'commented' => null]],
$container->getParameter('doctrine.dbal.connection_factory.types')
);
$this->assertEquals('%doctrine.dbal.connection_factory.types%', $container->getDefinition('doctrine.dbal.connection_factory')->getArgument(0));
Expand Down
24 changes: 24 additions & 0 deletions Tests/DependencyInjection/TestCommentedType.php
View file
@@ -0,0 +1,24 @@
<?php

namespace Doctrine\Bundle\DoctrineBundle\Tests\DependencyInjection;

use Doctrine\DBAL\Platforms\AbstractPlatform;
use Doctrine\DBAL\Types\Type;

class TestCommentedType extends Type
{
public function getName()
{
return 'test';
}

public function getSQLDeclaration(array $fieldDeclaration, AbstractPlatform $platform)
{
return '';
}

public function requiresSQLCommentHint(AbstractPlatform $platform)
{
return true;
}
}
5 changes: 4 additions & 1 deletion Tests/TestCase.php
View file
Expand Up @@ -48,7 +48,10 @@ public function createYamlBundleTestContainer()
],
'default_connection' => 'default',
'types' => [
'test' => TestType::class,
'test' => [
'class' => TestType::class,
'commented' => false,
],
],
], 'orm' => [
'default_entity_manager' => 'default',
Expand Down
4 changes: 4 additions & 0 deletions phpunit.xml.dist
View file
Expand Up @@ -17,4 +17,8 @@
</exclude>
</whitelist>
</filter>

<listeners>
<listener class="Symfony\Bridge\PhpUnit\SymfonyTestsListener" />
</listeners>
</phpunit>

0 comments on commit 5c79bbc

Please sign in to comment.