From 4ff891e081467fe123f6d6fb907bd4a73b5c115d Mon Sep 17 00:00:00 2001 From: Andreas Leathley Date: Wed, 25 Nov 2020 15:07:27 +0100 Subject: [PATCH] Better support typed properties Also removes compatibility to diverging property paths defined in the form, we only support direct properties which are mapped 1:1 in the form. This seems easiest and anything else would just lead to a headache, although maybe there is a better option sometime in the future or when we go PHP 8 only. The PHP 8 support is preliminary, as it was not tested yet. --- composer.json | 2 +- psalm-baseline.xml | 12 +- src/Annotation/StringFilterExtension.php | 88 ++++++++++++-- tests/FormExtensionTest.php | 111 ++++++++++++++++++ .../ClassWithPrivateTypedProperties.php | 40 +++++++ .../ClassWithPublicTypedProperties.php | 23 ++++ .../TestForms/PrivateTypedPropertiesForm.php | 36 ++++++ .../PublicTypedPropertiesEmptyDataForm.php | 41 +++++++ tests/TestForms/PublicTypedPropertiesForm.php | 40 +++++++ 9 files changed, 375 insertions(+), 18 deletions(-) create mode 100644 tests/TestClasses/ClassWithPrivateTypedProperties.php create mode 100644 tests/TestClasses/ClassWithPublicTypedProperties.php create mode 100644 tests/TestForms/PrivateTypedPropertiesForm.php create mode 100644 tests/TestForms/PublicTypedPropertiesEmptyDataForm.php create mode 100644 tests/TestForms/PublicTypedPropertiesForm.php diff --git a/composer.json b/composer.json index e19abc7..dee75b2 100644 --- a/composer.json +++ b/composer.json @@ -52,7 +52,7 @@ "phpstan": "vendor/bin/phpstan analyse", "phpstan_base": "vendor/bin/phpstan analyse --generate-baseline", "phpstan_clear": "vendor/bin/phpstan clear-result-cache", - "psalm": "vendor/bin/psalm --show-info=false --diff", + "psalm": "vendor/bin/psalm --show-info=false", "psalm_full": "vendor/bin/psalm --show-info=false", "psalm_base": "vendor/bin/psalm --set-baseline=psalm-baseline.xml", "phpunit": "vendor/bin/phpunit --colors=always", diff --git a/psalm-baseline.xml b/psalm-baseline.xml index a956765..14e8d42 100644 --- a/psalm-baseline.xml +++ b/psalm-baseline.xml @@ -1,8 +1,12 @@ - + - - $model - + + + $reflectionPropertyType + $reflectionPropertyType + \ReflectionUnionType + \ReflectionUnionType + diff --git a/src/Annotation/StringFilterExtension.php b/src/Annotation/StringFilterExtension.php index 57a3c03..4b38dd7 100644 --- a/src/Annotation/StringFilterExtension.php +++ b/src/Annotation/StringFilterExtension.php @@ -7,7 +7,6 @@ use Symfony\Component\Form\FormBuilderInterface; use Symfony\Component\Form\FormEvent; use Symfony\Component\Form\FormEvents; -use Symfony\Component\PropertyAccess\PropertyAccess; /** * Apply StringFilter annotations and filter submitted data accordingly @@ -37,9 +36,6 @@ public function buildForm(FormBuilderInterface $builder, array $options): void // We only want form elements with a data class and an array of values if (\is_array($data) && \strlen($options['data_class']) > 0) { - // Property accessor like the one used by the form component - $propertyAccessor = PropertyAccess::createPropertyAccessor(); - // Create instance of the form data object, either from empty_data or by instantiating it if (isset($options['empty_data']) && $options['empty_data'] instanceof $options['data_class']) { $model = clone $options['empty_data']; @@ -47,13 +43,46 @@ public function buildForm(FormBuilderInterface $builder, array $options): void $model = (new $options['data_class']()); } - // Assign values to the model as the form would do it + // Assign values to the model only for direct properties foreach ($data as $key => $value) { - if ($form->has($key)) { - $propertyPath = $form->get($key)->getPropertyPath(); + if (\property_exists($model, $key)) { + $reflectionProperty = new \ReflectionProperty($model, $key); + $reflectionPropertyType = $reflectionProperty->getType(); + + // @codeCoverageIgnoreStart + if ( + PHP_VERSION_ID >= 80000 + && $reflectionPropertyType instanceof \ReflectionUnionType + ) { + $reflectionTypes = $reflectionPropertyType->getTypes(); + } else { + $reflectionTypes = [$reflectionPropertyType]; + } + // @codeCoverageIgnoreEnd + + $hasSupportedType = false; + + if (!$reflectionProperty->hasType()) { + $hasSupportedType = true; + } + + foreach ($reflectionTypes as $reflectionType) { + if (!($reflectionType instanceof \ReflectionNamedType)) { + continue; + } + + if ( + $reflectionType->getName() === 'string' + || $reflectionType->getName() === 'array' + ) { + $hasSupportedType = true; + break; + } + } - if (isset($propertyPath) && $propertyAccessor->isWritable($model, $propertyPath)) { - $propertyAccessor->setValue($model, $propertyPath, $value); + if ($hasSupportedType === true) { + $reflectionProperty->setAccessible(true); + $reflectionProperty->setValue($model, $value); } } } @@ -63,11 +92,44 @@ public function buildForm(FormBuilderInterface $builder, array $options): void // Copy back the processed data to the array foreach ($data as $key => $value) { - if ($form->has($key)) { - $propertyPath = $form->get($key)->getPropertyPath(); + if (\property_exists($model, $key)) { + $reflectionProperty = new \ReflectionProperty($model, $key); + $reflectionPropertyType = $reflectionProperty->getType(); + + // @codeCoverageIgnoreStart + if ( + PHP_VERSION_ID >= 80000 + && $reflectionPropertyType instanceof \ReflectionUnionType + ) { + $reflectionTypes = $reflectionPropertyType->getTypes(); + } else { + $reflectionTypes = [$reflectionPropertyType]; + } + // @codeCoverageIgnoreEnd + + $hasSupportedType = false; + + if (!$reflectionProperty->hasType()) { + $hasSupportedType = true; + } + + foreach ($reflectionTypes as $reflectionType) { + if (!($reflectionType instanceof \ReflectionNamedType)) { + continue; + } + + if ( + $reflectionType->getName() === 'string' + || $reflectionType->getName() === 'array' + ) { + $hasSupportedType = true; + break; + } + } - if (isset($propertyPath) && $propertyAccessor->isReadable($model, $propertyPath)) { - $data[$key] = $propertyAccessor->getValue($model, $propertyPath); + if ($hasSupportedType === true) { + $reflectionProperty->setAccessible(true); + $data[$key] = $reflectionProperty->getValue($model); } } } diff --git a/tests/FormExtensionTest.php b/tests/FormExtensionTest.php index 90e0265..815b7aa 100644 --- a/tests/FormExtensionTest.php +++ b/tests/FormExtensionTest.php @@ -10,10 +10,15 @@ use Squirrel\Strings\Filter\TrimFilter; use Squirrel\Strings\StringFilterSelector; use Squirrel\Strings\Tests\TestClasses\ClassWithPrivateProperties; +use Squirrel\Strings\Tests\TestClasses\ClassWithPrivateTypedProperties; use Squirrel\Strings\Tests\TestClasses\ClassWithPublicProperties; +use Squirrel\Strings\Tests\TestClasses\ClassWithPublicTypedProperties; use Squirrel\Strings\Tests\TestForms\PrivatePropertiesForm; +use Squirrel\Strings\Tests\TestForms\PrivateTypedPropertiesForm; use Squirrel\Strings\Tests\TestForms\PublicPropertiesEmptyDataForm; use Squirrel\Strings\Tests\TestForms\PublicPropertiesForm; +use Squirrel\Strings\Tests\TestForms\PublicTypedPropertiesEmptyDataForm; +use Squirrel\Strings\Tests\TestForms\PublicTypedPropertiesForm; use Symfony\Component\Form\Extension\Core\Type\SubmitType; use Symfony\Component\Form\Extension\HttpFoundation\HttpFoundationExtension; use Symfony\Component\Form\Forms; @@ -106,6 +111,83 @@ public function testPublicPropertiesEmptyDataSubmit() $this->assertEquals('a II I ADHSAHZUsd', $data->text); } + public function testPublicTypedPropertiesSubmit() + { + $data = new ClassWithPublicTypedProperties(); + $data->title = ' JOOOOPPPPPP '; + $data->texts = [ + ' cOrEcT ', + ' oMundo ', + ]; + + $request = Request::create( + 'https://127.0.0.1/', // URI + 'POST', // method + [ // post parameters + 'public_typed_properties_form' => [ + 'title' => ' alsdjl DASDAD ', + 'texts' => [ + ' a II I ADHSAHZUsd ', + ' oMundo ', + ], + ], + ], + [], // cookies + [], // files + [], // $_SERVER + '' // content + ); + + $form = $this->formFactory->create(PublicTypedPropertiesForm::class, $data) + ->add('save', SubmitType::class, [ + 'label' => 'Save', + ]); + $form->handleRequest($request); + + $this->assertEquals('alsdjl dasdad', $data->title); + $this->assertEquals([ + 'a II I ADHSAHZUsd', + 'oMundo', + ], $data->texts); + } + + public function testPublicTypedPropertiesEmptyDataSubmit() + { + $request = Request::create( + 'https://127.0.0.1/', // URI + 'POST', // method + [ // post parameters + 'public_typed_properties_empty_data_form' => [ + 'title' => ' alsdjl DASDAD ', + 'texts' => [ + ' a II I ADHSAHZUsd ', + ' oMundo ', + ' ladidida ' . "\n", + ], + ], + ], + [], // cookies + [], // files + [], // $_SERVER + '' // content + ); + + $form = $this->formFactory->create(PublicTypedPropertiesEmptyDataForm::class) + ->add('save', SubmitType::class, [ + 'label' => 'Save', + ]); + $form->handleRequest($request); + + $data = $form->getData(); + + $this->assertEquals('alsdjl dasdad', $data->title); + $this->assertEquals([ + 'a II I ADHSAHZUsd', + 'oMundo', + 'ladidida', + ], $data->texts); + } + public function testPrivatePropertiesSubmit() { $data = new ClassWithPrivateProperties(); @@ -134,4 +216,33 @@ public function testPrivatePropertiesSubmit() $this->assertEquals('alsdjl dasdad', $data->getTitle()); $this->assertEquals('a II I ADHSAHZUsd', $data->getText()); } + + public function testPrivateTypedPropertiesSubmit() + { + $data = new ClassWithPrivateTypedProperties(); + + $request = Request::create( + 'https://127.0.0.1/', // URI + 'POST', // method + [ // post parameters + 'private_typed_properties_form' => [ + 'title' => ' alsdjl DASDAD ', + 'text' => ' a II I ADHSAHZUsd ', + ], + ], + [], // cookies + [], // files + [], // $_SERVER + '' // content + ); + + $form = $this->formFactory->create(PrivateTypedPropertiesForm::class, $data) + ->add('save', SubmitType::class, [ + 'label' => 'Save', + ]); + $form->handleRequest($request); + + $this->assertEquals('alsdjl dasdad', $data->getTitle()); + $this->assertEquals('a II I ADHSAHZUsd', $data->getText()); + } } diff --git a/tests/TestClasses/ClassWithPrivateTypedProperties.php b/tests/TestClasses/ClassWithPrivateTypedProperties.php new file mode 100644 index 0000000..39cf9e8 --- /dev/null +++ b/tests/TestClasses/ClassWithPrivateTypedProperties.php @@ -0,0 +1,40 @@ +title; + } + + public function getText(): string + { + return $this->text; + } + + public function setTitle(string $title) + { + $this->title = $title; + } + + public function setText(string $text) + { + $this->text = $text; + } +} diff --git a/tests/TestClasses/ClassWithPublicTypedProperties.php b/tests/TestClasses/ClassWithPublicTypedProperties.php new file mode 100644 index 0000000..5236b72 --- /dev/null +++ b/tests/TestClasses/ClassWithPublicTypedProperties.php @@ -0,0 +1,23 @@ +add('title', TextType::class, [ + 'label' => false, + ]) + ->add('text', TextType::class, [ + 'label' => false, + ]) + ; + } + + /** + * Set class where our form data comes from + * + * @param OptionsResolver $resolver + */ + public function configureOptions(OptionsResolver $resolver) + { + $resolver->setDefaults(array( + 'data_class' => ClassWithPrivateTypedProperties::class, + )); + } +} diff --git a/tests/TestForms/PublicTypedPropertiesEmptyDataForm.php b/tests/TestForms/PublicTypedPropertiesEmptyDataForm.php new file mode 100644 index 0000000..53b0623 --- /dev/null +++ b/tests/TestForms/PublicTypedPropertiesEmptyDataForm.php @@ -0,0 +1,41 @@ +add('title', TextType::class, [ + 'label' => false, + ]) + ->add('texts', CollectionType::class, [ + 'entry_type' => TextType::class, + 'allow_add' => true, + 'allow_delete' => true, + 'label' => false, + ]) + ; + } + + /** + * Set class where our form data comes from + * + * @param OptionsResolver $resolver + */ + public function configureOptions(OptionsResolver $resolver) + { + $resolver->setDefaults(array( + 'data_class' => ClassWithPublicTypedProperties::class, + 'empty_data' => new ClassWithPublicTypedProperties(), + )); + } +} diff --git a/tests/TestForms/PublicTypedPropertiesForm.php b/tests/TestForms/PublicTypedPropertiesForm.php new file mode 100644 index 0000000..9fbd85a --- /dev/null +++ b/tests/TestForms/PublicTypedPropertiesForm.php @@ -0,0 +1,40 @@ +add('title', TextType::class, [ + 'label' => false, + ]) + ->add('texts', CollectionType::class, [ + 'entry_type' => TextType::class, + 'allow_add' => true, + 'allow_delete' => true, + 'label' => false, + ]) + ; + } + + /** + * Set class where our form data comes from + * + * @param OptionsResolver $resolver + */ + public function configureOptions(OptionsResolver $resolver) + { + $resolver->setDefaults(array( + 'data_class' => ClassWithPublicTypedProperties::class, + )); + } +}