From 7ca2f0b753a5f8372dfc7c669dfd439617602030 Mon Sep 17 00:00:00 2001 From: Andreas Braun Date: Mon, 20 Jul 2020 14:59:47 +0200 Subject: [PATCH 1/3] [Form] Add accessor data mapper for built-in DTO support --- .../Form/Extension/Core/CoreExtension.php | 2 + .../Core/DataMapper/AccessorMapper.php | 81 +++++++ .../Core/Type/AccessorMapperExtension.php | 53 +++++ .../Extension/Core/CoreExtensionTest.php | 32 +++ .../Core/DataMapper/AccessorMapperTest.php | 224 ++++++++++++++++++ 5 files changed, 392 insertions(+) create mode 100644 src/Symfony/Component/Form/Extension/Core/DataMapper/AccessorMapper.php create mode 100644 src/Symfony/Component/Form/Extension/Core/Type/AccessorMapperExtension.php create mode 100644 src/Symfony/Component/Form/Tests/Extension/Core/DataMapper/AccessorMapperTest.php diff --git a/src/Symfony/Component/Form/Extension/Core/CoreExtension.php b/src/Symfony/Component/Form/Extension/Core/CoreExtension.php index 04e9dd45861f4..d3418bfe52ad3 100644 --- a/src/Symfony/Component/Form/Extension/Core/CoreExtension.php +++ b/src/Symfony/Component/Form/Extension/Core/CoreExtension.php @@ -16,6 +16,7 @@ use Symfony\Component\Form\ChoiceList\Factory\ChoiceListFactoryInterface; use Symfony\Component\Form\ChoiceList\Factory\DefaultChoiceListFactory; use Symfony\Component\Form\ChoiceList\Factory\PropertyAccessDecorator; +use Symfony\Component\Form\Extension\Core\Type\AccessorMapperExtension; use Symfony\Component\Form\Extension\Core\Type\TransformationFailureExtension; use Symfony\Component\PropertyAccess\PropertyAccess; use Symfony\Component\PropertyAccess\PropertyAccessorInterface; @@ -84,6 +85,7 @@ protected function loadTypeExtensions() { return [ new TransformationFailureExtension($this->translator), + new AccessorMapperExtension(), ]; } } diff --git a/src/Symfony/Component/Form/Extension/Core/DataMapper/AccessorMapper.php b/src/Symfony/Component/Form/Extension/Core/DataMapper/AccessorMapper.php new file mode 100644 index 0000000000000..ea29b816256ac --- /dev/null +++ b/src/Symfony/Component/Form/Extension/Core/DataMapper/AccessorMapper.php @@ -0,0 +1,81 @@ +get = $get; + $this->set = $set; + $this->fallbackMapper = $fallbackMapper; + } + + /** + * {@inheritdoc} + */ + public function mapDataToForms($data, iterable $forms) + { + $empty = null === $data || [] === $data; + + if (!$empty && !\is_array($data) && !\is_object($data)) { + throw new UnexpectedTypeException($data, 'object, array or empty'); + } + + if (!$this->get) { + $this->fallbackMapper->mapDataToForms($data, $forms); + return; + } + + foreach ($forms as $form) { + $config = $form->getConfig(); + + if (!$empty && $config->getMapped()) { + $form->setData($this->getPropertyValue($data)); + } else { + $form->setData($config->getData()); + } + } + } + + /** + * {@inheritdoc} + */ + public function mapFormsToData(iterable $forms, &$data) + { + if (null === $data) { + return; + } + + if (!\is_array($data) && !\is_object($data)) { + throw new UnexpectedTypeException($data, 'object, array or empty'); + } + + if (!$this->set) { + $this->fallbackMapper->mapFormsToData($forms, $data); + return; + } + + foreach ($forms as $form) { + $config = $form->getConfig(); + + // Write-back is disabled if the form is not synchronized (transformation failed), + // if the form was not submitted and if the form is disabled (modification not allowed) + if (null !== $this->set && $config->getMapped() && $form->isSubmitted() && $form->isSynchronized() && !$form->isDisabled()) { + ($this->set)($data, $form->getData()); + } + } + } + + private function getPropertyValue($data) + { + return $this->get ? ($this->get)($data) : null; + } +} diff --git a/src/Symfony/Component/Form/Extension/Core/Type/AccessorMapperExtension.php b/src/Symfony/Component/Form/Extension/Core/Type/AccessorMapperExtension.php new file mode 100644 index 0000000000000..dc29f5fcbe0fc --- /dev/null +++ b/src/Symfony/Component/Form/Extension/Core/Type/AccessorMapperExtension.php @@ -0,0 +1,53 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Symfony\Component\Form\Extension\Core\Type; + +use Closure; +use Symfony\Component\Form\AbstractTypeExtension; +use Symfony\Component\Form\Extension\Core\DataMapper\AccessorMapper; +use Symfony\Component\Form\FormBuilderInterface; +use Symfony\Component\OptionsResolver\OptionsResolver; + +class AccessorMapperExtension extends AbstractTypeExtension +{ + public function buildForm(FormBuilderInterface $builder, array $options) + { + if (!$options['get'] && !$options['set']) { + return; + } + + if (!$dataMapper = $builder->getDataMapper()) { + return; + } + + $builder->setDataMapper(new AccessorMapper($options['get'], $options['set'], $dataMapper)); + } + + public function configureOptions(OptionsResolver $resolver) + { + $resolver->setDefaults([ + 'get' => null, + 'set' => null, + ]); + + $resolver->setAllowedTypes('get', ['null', Closure::class]); + $resolver->setAllowedTypes('set', ['null', Closure::class]); + } + + /** + * {@inheritdoc} + */ + public static function getExtendedTypes(): iterable + { + return [FormType::class]; + } +} diff --git a/src/Symfony/Component/Form/Tests/Extension/Core/CoreExtensionTest.php b/src/Symfony/Component/Form/Tests/Extension/Core/CoreExtensionTest.php index ff85149e21c63..63176d710878a 100644 --- a/src/Symfony/Component/Form/Tests/Extension/Core/CoreExtensionTest.php +++ b/src/Symfony/Component/Form/Tests/Extension/Core/CoreExtensionTest.php @@ -11,8 +11,12 @@ namespace Symfony\Component\Form\Tests\Extension\Core; +use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; +use stdClass; use Symfony\Component\Form\Extension\Core\CoreExtension; +use Symfony\Component\Form\Extension\Core\DataMapper\AccessorMapper; +use Symfony\Component\Form\Extension\Core\Type\TextType; use Symfony\Component\Form\FormFactoryBuilder; class CoreExtensionTest extends TestCase @@ -30,4 +34,32 @@ public function testTransformationFailuresAreConvertedIntoFormErrors() $this->assertFalse($form->isValid()); } + + public function testMapperExtensionIsLoaded() + { + $formFactoryBuilder = new FormFactoryBuilder(); + $formFactory = $formFactoryBuilder->addExtension(new CoreExtension()) + ->getFormFactory(); + + $mock = $this->getMockBuilder(stdClass::class)->addMethods(['get', 'set'])->getMock(); + $mock->expects($this->once())->method('get')->willReturn('foo'); + $mock->expects($this->once())->method('set')->with('bar'); + + $formBuilder = $formFactory->createBuilder(); + $form = $formBuilder + ->add( + 'foo', + TextType::class + ) + ->setDataMapper(new AccessorMapper( + function (MockObject $data) { return $data->get(); }, + function (MockObject $data, $value) { return $data->set($value); }, + $formBuilder->getDataMapper() + )) + ->setData($mock) + ->getForm(); + + $this->assertInstanceOf(AccessorMapper::class, $form->getConfig()->getDataMapper()); + $form->submit(['foo' => 'bar']); + } } diff --git a/src/Symfony/Component/Form/Tests/Extension/Core/DataMapper/AccessorMapperTest.php b/src/Symfony/Component/Form/Tests/Extension/Core/DataMapper/AccessorMapperTest.php new file mode 100644 index 0000000000000..b8d5a0358863a --- /dev/null +++ b/src/Symfony/Component/Form/Tests/Extension/Core/DataMapper/AccessorMapperTest.php @@ -0,0 +1,224 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Symfony\Component\Form\Tests\Extension\Core\DataMapper; + +use PHPUnit\Framework\MockObject\Matcher\Invocation; +use PHPUnit\Framework\TestCase; +use stdClass; +use Symfony\Component\EventDispatcher\EventDispatcher; +use Symfony\Component\EventDispatcher\EventDispatcherInterface; +use Symfony\Component\Form\DataMapperInterface; +use Symfony\Component\Form\Extension\Core\DataMapper\AccessorMapper; +use Symfony\Component\Form\Extension\Core\DataMapper\PropertyPathMapper; +use Symfony\Component\Form\Form; +use Symfony\Component\Form\FormConfigBuilder; +use Symfony\Component\PropertyAccess\PropertyAccess; +use Symfony\Component\PropertyAccess\PropertyAccessorInterface; +use Symfony\Component\PropertyAccess\PropertyPath; + +class AccessorMapperTest extends TestCase +{ + /** + * @var EventDispatcherInterface + */ + private $dispatcher; + + /** + * @var PropertyPathMapper + */ + private $propertyPathMapper; + + /** + * @var PropertyAccessorInterface + */ + private $propertyAccessor; + + protected function setUp(): void + { + $this->dispatcher = new EventDispatcher(); + $this->propertyAccessor = PropertyAccess::createPropertyAccessor(); + $this->propertyPathMapper = $this->getMockBuilder(DataMapperInterface::class)->getMock(); + } + + public function testGetUsesFallbackIfNoAccessor() + { + $this->setupPropertyPathMapper($this->once(), $this->never()); + + $data = $this->getMockBuilder(stdClass::class)->addMethods(['getEngine', 'getEngineClosure'])->getMock(); + $data + ->expects($this->never()) + ->method('getEngineClosure'); + $data + ->expects($this->once()) + ->method('getEngine') + ->willReturn('electric'); + + $propertyPath = new PropertyPath('engine'); + + $config = new FormConfigBuilder('name', null, $this->dispatcher); + $config->setByReference(true); + $config->setPropertyPath($propertyPath); + $form = new Form($config); + + $mapper = $this->createMapper(false, false); + $mapper->mapDataToForms($data, [$form]); + + $this->assertSame('electric', $form->getData()); + } + + public function testGetUsesAccessor() + { + $this->setupPropertyPathMapper($this->never(), $this->never()); + + $data = $this->getMockBuilder(stdClass::class)->addMethods(['getEngine', 'getEngineClosure'])->getMock(); + $data + ->expects($this->once()) + ->method('getEngineClosure') + ->willReturn('electric'); + $data + ->expects($this->never()) + ->method('getEngine'); + + $propertyPath = new PropertyPath('engine'); + + $config = new FormConfigBuilder('name', null, $this->dispatcher); + $config->setByReference(true); + $config->setPropertyPath($propertyPath); + $form = new Form($config); + + $mapper = $this->createMapper(true, false); + $mapper->mapDataToForms($data, [$form]); + + $this->assertSame('electric', $form->getData()); + } + + public function testSetUsesAccessor() + { + $this->setupPropertyPathMapper($this->never(), $this->never()); + + $data = new class() { + private $engine; + + public function setEngineClosure($engine) + { + $this->engine = $engine; + } + + public function getEngine() + { + return $this->engine; + } + }; + + $propertyPath = new PropertyPath('engine'); + + $config = new FormConfigBuilder('name', null, $this->dispatcher); + $config->setByReference(true); + $config->setPropertyPath($propertyPath); + $form = new Form($config); + + $form->submit('electric'); + $mapper = $this->createMapper(false, true); + $mapper->mapFormsToData([$form], $data); + + $this->assertSame('electric', $data->getEngine()); + } + + public function testSetUsesAccessorForCompoundFields() + { + $this->setupPropertyPathMapper($this->any(), $this->any()); + + $data = new class() { + private $foo; + private $bar; + + public function setEngineClosure($data) + { + if (!$data) { + return; + } + + foreach ($data as $key => $value) { + $this->$key = $value; + } + } + + public function getEngineClosure() + { + return [ + 'foo' => $this->foo, + 'bar' => $this->bar, + ]; + } + + public function getFoo() + { + return $this->foo; + } + + public function getBar() + { + return $this->bar; + } + }; + + $config = new FormConfigBuilder('address', null, $this->dispatcher); + $config->setCompound(true); + $config->setDataMapper(new PropertyPathMapper($this->propertyAccessor)); + $addressForm = new Form($config); + $addressForm + ->add(new Form(new FormConfigBuilder('foo', null, $this->dispatcher))) + ->add(new Form(new FormConfigBuilder('bar', null, $this->dispatcher))); + + $mapper = $this->createMapper(true, true); + + $config = new FormConfigBuilder('name', null, $this->dispatcher); + $config->setCompound(true); + $config->setDataMapper($mapper); + $config->setData($data); + $form = new Form($config); + $form->add($addressForm); + + $form->submit(['address' => ['foo' => 'foo', 'bar' => 'bar']]); + $this->assertNull($form->getTransformationFailure()); + + $this->assertSame('foo', $data->getFoo()); + $this->assertSame('bar', $data->getBar()); + } + + private function setupPropertyPathMapper(Invocation $dataToFormsMatcher, Invocation $formsToDataMatcher): void + { + $propertyPathMapper = new PropertyPathMapper($this->propertyAccessor); + + $this->propertyPathMapper + ->expects($dataToFormsMatcher) + ->method('mapDataToForms') + ->willReturnCallback(function (...$args) use ($propertyPathMapper) { + $propertyPathMapper->mapDataToForms(...$args); + }); + $this->propertyPathMapper + ->expects($formsToDataMatcher) + ->method('mapFormsToData') + ->willReturnCallback(function (...$args) use ($propertyPathMapper) { + $propertyPathMapper->mapFormsToData(...$args); + }); + } + + private function createMapper(bool $useGetClosure, bool $useSetClosure) + { + return new AccessorMapper( + $useGetClosure ? function ($object) { return $object->getEngineClosure(); } : null, + $useSetClosure ? function ($object, $value) { $object->setEngineClosure($value); } : null, + $this->propertyPathMapper + ); + } +} From 2055af53d8cd448d32a4606c6c29bd4492216e9e Mon Sep 17 00:00:00 2001 From: Andreas Braun Date: Mon, 20 Jul 2020 16:02:15 +0200 Subject: [PATCH 2/3] [Form] Support immutable objects in AccessorMapper --- .../Core/DataMapper/AccessorMapper.php | 10 ++++- .../Core/DataMapper/AccessorMapperTest.php | 43 ++++++++++++++++++- 2 files changed, 51 insertions(+), 2 deletions(-) diff --git a/src/Symfony/Component/Form/Extension/Core/DataMapper/AccessorMapper.php b/src/Symfony/Component/Form/Extension/Core/DataMapper/AccessorMapper.php index ea29b816256ac..01b0985133271 100644 --- a/src/Symfony/Component/Form/Extension/Core/DataMapper/AccessorMapper.php +++ b/src/Symfony/Component/Form/Extension/Core/DataMapper/AccessorMapper.php @@ -69,7 +69,15 @@ public function mapFormsToData(iterable $forms, &$data) // Write-back is disabled if the form is not synchronized (transformation failed), // if the form was not submitted and if the form is disabled (modification not allowed) if (null !== $this->set && $config->getMapped() && $form->isSubmitted() && $form->isSynchronized() && !$form->isDisabled()) { - ($this->set)($data, $form->getData()); + $returnValue = ($this->set)($data, $form->getData()); + $type = is_object($returnValue) ? get_class($returnValue) : gettype($returnValue); + + if ( + (is_scalar($data) && gettype($data) === $type) + || (is_array($data) && is_array($returnValue)) + || (is_object($data) && $returnValue instanceof $type)) { + $data = $returnValue; + } } } } diff --git a/src/Symfony/Component/Form/Tests/Extension/Core/DataMapper/AccessorMapperTest.php b/src/Symfony/Component/Form/Tests/Extension/Core/DataMapper/AccessorMapperTest.php index b8d5a0358863a..d9ed68de51e15 100644 --- a/src/Symfony/Component/Form/Tests/Extension/Core/DataMapper/AccessorMapperTest.php +++ b/src/Symfony/Component/Form/Tests/Extension/Core/DataMapper/AccessorMapperTest.php @@ -195,6 +195,47 @@ public function getBar() $this->assertSame('bar', $data->getBar()); } + public function testSetAccessorSupportsImmutableObjects() + { + $this->setupPropertyPathMapper($this->any(), $this->any()); + + $data = new class('petrol') { + private $engine; + + public function __construct(string $engine) + { + $this->engine = $engine; + } + + public function setEngineClosure($data) + { + return new self($data); + } + + public function getEngineClosure() + { + return $this->engine; + } + }; + + $config = new FormConfigBuilder('car', null, $this->dispatcher); + $config->setCompound(true); + $config->setDataMapper($this->createMapper(true, true)); + $config->setData($data); + $form = new Form($config); + $form + ->add(new Form(new FormConfigBuilder('engine', null, $this->dispatcher))); + + $form->submit(['engine' => 'electric']); + $this->assertNull($form->getTransformationFailure()); + + $formData = $form->getData(); + + $this->assertNotSame($data, $formData); + $this->assertSame('electric', $formData->getEngineClosure()); + $this->assertSame('petrol', $data->getEngineClosure()); + } + private function setupPropertyPathMapper(Invocation $dataToFormsMatcher, Invocation $formsToDataMatcher): void { $propertyPathMapper = new PropertyPathMapper($this->propertyAccessor); @@ -217,7 +258,7 @@ private function createMapper(bool $useGetClosure, bool $useSetClosure) { return new AccessorMapper( $useGetClosure ? function ($object) { return $object->getEngineClosure(); } : null, - $useSetClosure ? function ($object, $value) { $object->setEngineClosure($value); } : null, + $useSetClosure ? function ($object, $value) { return $object->setEngineClosure($value); } : null, $this->propertyPathMapper ); } From 2fc85b01a3520bea54bbb61a3b15c0b0c9ef76b5 Mon Sep 17 00:00:00 2001 From: Andreas Braun Date: Mon, 20 Jul 2020 17:05:28 +0200 Subject: [PATCH 3/3] [Form] Support catching exceptions from an accessor --- .../Core/DataMapper/AccessorMapper.php | 11 +++- .../Core/DataMapper/AccessorMapperTest.php | 61 +++++++++++++++++++ 2 files changed, 71 insertions(+), 1 deletion(-) diff --git a/src/Symfony/Component/Form/Extension/Core/DataMapper/AccessorMapper.php b/src/Symfony/Component/Form/Extension/Core/DataMapper/AccessorMapper.php index 01b0985133271..35bcc33e62861 100644 --- a/src/Symfony/Component/Form/Extension/Core/DataMapper/AccessorMapper.php +++ b/src/Symfony/Component/Form/Extension/Core/DataMapper/AccessorMapper.php @@ -3,7 +3,10 @@ namespace Symfony\Component\Form\Extension\Core\DataMapper; use Symfony\Component\Form\DataMapperInterface; +use Symfony\Component\Form\Exception\ExceptionInterface; use Symfony\Component\Form\Exception\UnexpectedTypeException; +use Symfony\Component\Form\FormError; +use TypeError; class AccessorMapper implements DataMapperInterface { @@ -69,7 +72,13 @@ public function mapFormsToData(iterable $forms, &$data) // Write-back is disabled if the form is not synchronized (transformation failed), // if the form was not submitted and if the form is disabled (modification not allowed) if (null !== $this->set && $config->getMapped() && $form->isSubmitted() && $form->isSynchronized() && !$form->isDisabled()) { - $returnValue = ($this->set)($data, $form->getData()); + try { + $returnValue = ($this->set)($data, $form->getData()); + } catch (ExceptionInterface | TypeError $e) { + $form->addError(new FormError($e->getMessage())); + continue; + } + $type = is_object($returnValue) ? get_class($returnValue) : gettype($returnValue); if ( diff --git a/src/Symfony/Component/Form/Tests/Extension/Core/DataMapper/AccessorMapperTest.php b/src/Symfony/Component/Form/Tests/Extension/Core/DataMapper/AccessorMapperTest.php index d9ed68de51e15..e868be5b3484f 100644 --- a/src/Symfony/Component/Form/Tests/Extension/Core/DataMapper/AccessorMapperTest.php +++ b/src/Symfony/Component/Form/Tests/Extension/Core/DataMapper/AccessorMapperTest.php @@ -1,5 +1,8 @@ assertSame('petrol', $data->getEngineClosure()); } + public static function invalidValueProvider(): \Generator + { + yield 'validation error' => ['#Corn is not a valid engine type#', 'corn']; + yield 'type error' => ['#Argument 1 passed to class@anonymous::setEngineClosure\(\) must be of the type string, object given#', new stdClass()]; + } + + /** + * @dataProvider InvalidValueProvider + */ + public function testSetAccessorCatchesExceptions(string $errorMessagePattern, $value) + { + $this->setupPropertyPathMapper($this->any(), $this->any()); + + $data = new class('petrol') { + private $engine; + + public function __construct(string $engine) + { + $this->engine = $engine; + } + + public function setEngineClosure(string $data) + { + if ($data === 'corn') { + throw new class extends RuntimeException + { + public function __construct() + { + parent::__construct('Corn is not a valid engine type'); + } + }; + } + + $this->engine = $data; + } + + public function getEngineClosure() + { + return $this->engine; + } + }; + + $config = new FormConfigBuilder('car', null, $this->dispatcher); + $config->setCompound(true); + $config->setDataMapper($this->createMapper(true, true)); + $config->setData($data); + $form = new Form($config); + $form + ->add(new Form(new FormConfigBuilder('engine', null, $this->dispatcher))); + + $form->submit(['engine' => $value]); + $this->assertFalse($form->isValid()); + $this->assertSame('petrol', $data->getEngineClosure()); + + $this->assertMatchesRegularExpression($errorMessagePattern, (string) $form->get('engine')->getErrors()); + } + private function setupPropertyPathMapper(Invocation $dataToFormsMatcher, Invocation $formsToDataMatcher): void { $propertyPathMapper = new PropertyPathMapper($this->propertyAccessor);