From e26893b1220ec04811e769ff3e5ac408c855e410 Mon Sep 17 00:00:00 2001 From: "Alexander M. Turek" Date: Mon, 30 Nov 2020 22:18:20 +0100 Subject: [PATCH 1/4] [DependencyInjection] Fix container linter for union types. --- .github/patch-types.php | 1 + .../Compiler/CheckTypeDeclarationsPass.php | 29 ++++---- .../CheckTypeDeclarationsPassTest.php | 69 +++++++++++++++++++ .../UnionConstructor.php | 10 +++ 4 files changed, 95 insertions(+), 14 deletions(-) create mode 100644 src/Symfony/Component/DependencyInjection/Tests/Fixtures/CheckTypeDeclarationsPass/UnionConstructor.php diff --git a/.github/patch-types.php b/.github/patch-types.php index 311e9e7ee4..d3dfc9073d 100644 --- a/.github/patch-types.php +++ b/.github/patch-types.php @@ -30,6 +30,7 @@ foreach ($loader->getClassMap() as $class => $file) { case false !== strpos($file, '/src/Symfony/Component/Config/Tests/Fixtures/ParseError.php'): case false !== strpos($file, '/src/Symfony/Component/Debug/Tests/Fixtures/'): case false !== strpos($file, '/src/Symfony/Component/DependencyInjection/Tests/Compiler/OptionalServiceClass.php'): + case false !== strpos($file, '/src/Symfony/Component/DependencyInjection/Tests/Fixtures/CheckTypeDeclarationsPass/UnionConstructor.php'): case false !== strpos($file, '/src/Symfony/Component/DependencyInjection/Tests/Fixtures/includes/autowiring_classes.php'): case false !== strpos($file, '/src/Symfony/Component/DependencyInjection/Tests/Fixtures/includes/uniontype_classes.php'): case false !== strpos($file, '/src/Symfony/Component/DependencyInjection/Tests/Fixtures/ParentNotExists.php'): diff --git a/src/Symfony/Component/DependencyInjection/Compiler/CheckTypeDeclarationsPass.php b/src/Symfony/Component/DependencyInjection/Compiler/CheckTypeDeclarationsPass.php index ae1a2ec204..752633ce17 100644 --- a/src/Symfony/Component/DependencyInjection/Compiler/CheckTypeDeclarationsPass.php +++ b/src/Symfony/Component/DependencyInjection/Compiler/CheckTypeDeclarationsPass.php @@ -153,26 +153,27 @@ final class CheckTypeDeclarationsPass extends AbstractRecursivePass /** * @throws InvalidParameterTypeException When a parameter is not compatible with the declared type */ - private function checkType(Definition $checkedDefinition, $value, \ReflectionParameter $parameter, ?string $envPlaceholderUniquePrefix, string $type = null): void + private function checkType(Definition $checkedDefinition, $value, \ReflectionParameter $parameter, ?string $envPlaceholderUniquePrefix, \ReflectionType $reflectionType = null): void { - if (null === $type) { - $type = $parameter->getType(); + $reflectionType = $reflectionType ?? $parameter->getType(); - if ($type instanceof \ReflectionUnionType) { - foreach ($type->getTypes() as $type) { - try { - $this->checkType($checkedDefinition, $value, $parameter, $envPlaceholderUniquePrefix, $type); + if ($reflectionType instanceof \ReflectionUnionType) { + foreach ($reflectionType->getTypes() as $t) { + try { + $this->checkType($checkedDefinition, $value, $parameter, $envPlaceholderUniquePrefix, $t); - return; - } catch (InvalidParameterTypeException $e) { - } + return; + } catch (InvalidParameterTypeException $e) { } - - throw new InvalidParameterTypeException($this->currentId, $e->getCode(), $parameter); } - $type = $type->getName(); + throw new InvalidParameterTypeException($this->currentId, $e->getCode(), $parameter); } + if (!$reflectionType instanceof \ReflectionNamedType) { + return; + } + + $type = $reflectionType->getName(); if ($value instanceof Reference) { if (!$this->container->has($value = (string) $value)) { @@ -285,7 +286,7 @@ final class CheckTypeDeclarationsPass extends AbstractRecursivePass $checkFunction = sprintf('is_%s', $type); - if (!$parameter->getType()->isBuiltin() || !$checkFunction($value)) { + if (!$reflectionType->isBuiltin() || !$checkFunction($value)) { throw new InvalidParameterTypeException($this->currentId, \is_object($value) ? $class : \gettype($value), $parameter); } } diff --git a/src/Symfony/Component/DependencyInjection/Tests/Compiler/CheckTypeDeclarationsPassTest.php b/src/Symfony/Component/DependencyInjection/Tests/Compiler/CheckTypeDeclarationsPassTest.php index 0842f26879..c683c3ed0a 100644 --- a/src/Symfony/Component/DependencyInjection/Tests/Compiler/CheckTypeDeclarationsPassTest.php +++ b/src/Symfony/Component/DependencyInjection/Tests/Compiler/CheckTypeDeclarationsPassTest.php @@ -26,6 +26,7 @@ use Symfony\Component\DependencyInjection\Tests\Fixtures\CheckTypeDeclarationsPa use Symfony\Component\DependencyInjection\Tests\Fixtures\CheckTypeDeclarationsPass\BarOptionalArgumentNotNull; use Symfony\Component\DependencyInjection\Tests\Fixtures\CheckTypeDeclarationsPass\Foo; use Symfony\Component\DependencyInjection\Tests\Fixtures\CheckTypeDeclarationsPass\FooObject; +use Symfony\Component\DependencyInjection\Tests\Fixtures\CheckTypeDeclarationsPass\UnionConstructor; use Symfony\Component\DependencyInjection\Tests\Fixtures\CheckTypeDeclarationsPass\Waldo; use Symfony\Component\DependencyInjection\Tests\Fixtures\CheckTypeDeclarationsPass\Wobble; use Symfony\Component\ExpressionLanguage\Expression; @@ -803,4 +804,72 @@ class CheckTypeDeclarationsPassTest extends TestCase putenv('ARRAY='); } + + /** + * @requires PHP 8 + */ + public function testUnionTypePassesWithReference() + { + $container = new ContainerBuilder(); + + $container->register('foo', Foo::class); + $container->register('union', UnionConstructor::class) + ->setArguments([new Reference('foo')]); + + (new CheckTypeDeclarationsPass(true))->process($container); + + $this->addToAssertionCount(1); + } + + /** + * @requires PHP 8 + */ + public function testUnionTypePassesWithBuiltin() + { + $container = new ContainerBuilder(); + + $container->register('union', UnionConstructor::class) + ->setArguments([42]); + + (new CheckTypeDeclarationsPass(true))->process($container); + + $this->addToAssertionCount(1); + } + + /** + * @requires PHP 8 + */ + public function testUnionTypeFailsWithReference() + { + $container = new ContainerBuilder(); + + $container->register('waldo', Waldo::class); + $container->register('union', UnionConstructor::class) + ->setArguments([new Reference('waldo')]); + + $this->expectException(\Symfony\Component\DependencyInjection\Exception\InvalidArgumentException::class); + $this->expectExceptionMessage('Invalid definition for service "union": argument 1 of "Symfony\\Component\\DependencyInjection\\Tests\\Fixtures\\CheckTypeDeclarationsPass\\UnionConstructor::__construct" accepts "Symfony\Component\DependencyInjection\Tests\Fixtures\CheckTypeDeclarationsPass\Foo|int", "Symfony\Component\DependencyInjection\Tests\Fixtures\CheckTypeDeclarationsPass\Waldo" passed.'); + + (new CheckTypeDeclarationsPass(true))->process($container); + + $this->addToAssertionCount(1); + } + + /** + * @requires PHP 8 + */ + public function testUnionTypeFailsWithBuiltin() + { + $container = new ContainerBuilder(); + + $container->register('union', UnionConstructor::class) + ->setArguments([[1, 2, 3]]); + + $this->expectException(\Symfony\Component\DependencyInjection\Exception\InvalidArgumentException::class); + $this->expectExceptionMessage('Invalid definition for service "union": argument 1 of "Symfony\\Component\\DependencyInjection\\Tests\\Fixtures\\CheckTypeDeclarationsPass\\UnionConstructor::__construct" accepts "Symfony\Component\DependencyInjection\Tests\Fixtures\CheckTypeDeclarationsPass\Foo|int", "array" passed.'); + + (new CheckTypeDeclarationsPass(true))->process($container); + + $this->addToAssertionCount(1); + } } diff --git a/src/Symfony/Component/DependencyInjection/Tests/Fixtures/CheckTypeDeclarationsPass/UnionConstructor.php b/src/Symfony/Component/DependencyInjection/Tests/Fixtures/CheckTypeDeclarationsPass/UnionConstructor.php new file mode 100644 index 0000000000..5ba5972863 --- /dev/null +++ b/src/Symfony/Component/DependencyInjection/Tests/Fixtures/CheckTypeDeclarationsPass/UnionConstructor.php @@ -0,0 +1,10 @@ + Date: Sat, 5 Dec 2020 17:39:56 +0100 Subject: [PATCH 2/4] [Config] YamlReferenceDumper: No default value required for VariableNode with array example --- .../Config/Definition/Dumper/YamlReferenceDumper.php | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/Symfony/Component/Config/Definition/Dumper/YamlReferenceDumper.php b/src/Symfony/Component/Config/Definition/Dumper/YamlReferenceDumper.php index e19e09ca7c..7f98d35bcc 100644 --- a/src/Symfony/Component/Config/Definition/Dumper/YamlReferenceDumper.php +++ b/src/Symfony/Component/Config/Definition/Dumper/YamlReferenceDumper.php @@ -17,6 +17,7 @@ use Symfony\Component\Config\Definition\EnumNode; use Symfony\Component\Config\Definition\NodeInterface; use Symfony\Component\Config\Definition\PrototypedArrayNode; use Symfony\Component\Config\Definition\ScalarNode; +use Symfony\Component\Config\Definition\VariableNode; use Symfony\Component\Yaml\Inline; /** @@ -95,6 +96,9 @@ class YamlReferenceDumper } elseif ($node instanceof EnumNode) { $comments[] = 'One of '.implode('; ', array_map('json_encode', $node->getValues())); $default = $node->hasDefaultValue() ? Inline::dump($node->getDefaultValue()) : '~'; + } elseif (VariableNode::class === get_class($node) && \is_array($example)) { + // If there is an array example, we are sure we dont need to print a default value + $default = ''; } else { $default = '~'; From 1f1b62afb64b2850f44a6c5eff93b602919747b4 Mon Sep 17 00:00:00 2001 From: "Alexander M. Turek" Date: Sat, 28 Nov 2020 23:52:30 +0100 Subject: [PATCH 3/4] [Messenger] Test generated SQL --- .../Transport/Doctrine/ConnectionTest.php | 56 ++++++++++++++++++- 1 file changed, 55 insertions(+), 1 deletion(-) diff --git a/src/Symfony/Component/Messenger/Tests/Transport/Doctrine/ConnectionTest.php b/src/Symfony/Component/Messenger/Tests/Transport/Doctrine/ConnectionTest.php index f297c02e82..ff87d9b512 100644 --- a/src/Symfony/Component/Messenger/Tests/Transport/Doctrine/ConnectionTest.php +++ b/src/Symfony/Component/Messenger/Tests/Transport/Doctrine/ConnectionTest.php @@ -12,9 +12,14 @@ namespace Symfony\Component\Messenger\Tests\Transport\Doctrine; use Doctrine\DBAL\Abstraction\Result as AbstractionResult; +use Doctrine\DBAL\Connection as DBALConnection; use Doctrine\DBAL\DBALException; +use Doctrine\DBAL\Driver\Result as DriverResult; +use Doctrine\DBAL\Driver\ResultStatement; use Doctrine\DBAL\Exception; use Doctrine\DBAL\Platforms\AbstractPlatform; +use Doctrine\DBAL\Platforms\MySQL57Platform; +use Doctrine\DBAL\Platforms\SQLServer2012Platform; use Doctrine\DBAL\Query\QueryBuilder; use Doctrine\DBAL\Result; use Doctrine\DBAL\Schema\AbstractSchemaManager; @@ -117,7 +122,7 @@ class ConnectionTest extends TestCase private function getDBALConnectionMock() { - $driverConnection = $this->createMock(\Doctrine\DBAL\Connection::class); + $driverConnection = $this->createMock(DBALConnection::class); $platform = $this->createMock(AbstractPlatform::class); $platform->method('getWriteLockSQL')->willReturn('FOR UPDATE'); $configuration = $this->createMock(\Doctrine\DBAL\Configuration::class); @@ -345,4 +350,53 @@ class ConnectionTest extends TestCase $this->assertEquals('{"message":"Hi again"}', $doctrineEnvelopes[1]['body']); $this->assertEquals(['type' => DummyMessage::class], $doctrineEnvelopes[1]['headers']); } + + /** + * @dataProvider providePlatformSql + */ + public function testGeneratedSql(AbstractPlatform $platform, string $expectedSql) + { + $driverConnection = $this->createMock(DBALConnection::class); + $driverConnection->method('getDatabasePlatform')->willReturn($platform); + $driverConnection->method('createQueryBuilder')->willReturnCallback(function () use ($driverConnection) { + return new QueryBuilder($driverConnection); + }); + + if (interface_exists(DriverResult::class)) { + $result = $this->createMock(DriverResult::class); + $result->method('fetchAssociative')->willReturn(false); + + if (class_exists(Result::class)) { + $result = new Result($result, $driverConnection); + } + } else { + $result = $this->createMock(ResultStatement::class); + $result->method('fetch')->willReturn(false); + } + + $driverConnection->expects($this->once())->method('beginTransaction'); + $driverConnection + ->expects($this->once()) + ->method('executeQuery') + ->with($expectedSql) + ->willReturn($result) + ; + $driverConnection->expects($this->once())->method('commit'); + + $connection = new Connection([], $driverConnection); + $connection->get(); + } + + public function providePlatformSql(): iterable + { + yield 'MySQL' => [ + new MySQL57Platform(), + 'SELECT m.* FROM messenger_messages m WHERE (m.delivered_at is null OR m.delivered_at < ?) AND (m.available_at <= ?) AND (m.queue_name = ?) ORDER BY available_at ASC LIMIT 1 FOR UPDATE', + ]; + + yield 'SQL Server' => [ + new SQLServer2012Platform(), + 'SELECT m.* FROM messenger_messages m WITH (UPDLOCK, ROWLOCK) WHERE (m.delivered_at is null OR m.delivered_at < ?) AND (m.available_at <= ?) AND (m.queue_name = ?) ORDER BY available_at ASC OFFSET 0 ROWS FETCH NEXT 1 ROWS ONLY ', + ]; + } } From fd3c60b31559934d73bf345c8fa19d9e4c11c6c2 Mon Sep 17 00:00:00 2001 From: Fabien Potencier Date: Sat, 5 Dec 2020 18:09:55 +0100 Subject: [PATCH 4/4] Fix CS --- .../Component/Config/Definition/Dumper/YamlReferenceDumper.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Symfony/Component/Config/Definition/Dumper/YamlReferenceDumper.php b/src/Symfony/Component/Config/Definition/Dumper/YamlReferenceDumper.php index 7f98d35bcc..c4baeccd18 100644 --- a/src/Symfony/Component/Config/Definition/Dumper/YamlReferenceDumper.php +++ b/src/Symfony/Component/Config/Definition/Dumper/YamlReferenceDumper.php @@ -96,7 +96,7 @@ class YamlReferenceDumper } elseif ($node instanceof EnumNode) { $comments[] = 'One of '.implode('; ', array_map('json_encode', $node->getValues())); $default = $node->hasDefaultValue() ? Inline::dump($node->getDefaultValue()) : '~'; - } elseif (VariableNode::class === get_class($node) && \is_array($example)) { + } elseif (VariableNode::class === \get_class($node) && \is_array($example)) { // If there is an array example, we are sure we dont need to print a default value $default = ''; } else {