Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Fix form/data mapping for typehinted properties #36492

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 7 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@

use Symfony\Component\Form\DataMapperInterface;
use Symfony\Component\Form\Exception\UnexpectedTypeException;
use Symfony\Component\PropertyAccess\Exception\AccessException;
use Symfony\Component\PropertyAccess\Exception\UninitializedPropertyException;
use Symfony\Component\PropertyAccess\PropertyAccess;
use Symfony\Component\PropertyAccess\PropertyAccessorInterface;

Expand Down Expand Up @@ -46,7 +48,12 @@ public function mapDataToForms($data, $forms)
$config = $form->getConfig();

if (!$empty && null !== $propertyPath && $config->getMapped()) {
$form->setData($this->propertyAccessor->getValue($data, $propertyPath));
try {
$form->setData($this->propertyAccessor->getValue($data, $propertyPath));
} catch (AccessException $e) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should either target 3.4 and/or catch the new UninitializedPropertyException.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed the target branch to 3.4.
I'm not sure if catching UninitializedPropertyException provides an advantage over the broader AccessException here. Is there a scenario in which catching the other possible AccessExceptions is harmful in the light of a flexible implementation?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The advantage is that the AccessException can be used internally by the PropertyAccess component for a "broader" usage, which should not be covered here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And yes it could lead to bug because the form should break as misconfigured so the dev can fix it, because a property is not readable/writable as it should.
That's why I think we really should consider master instead and consider this a new feature unlocked by the new exception (that was my original intent, but I'm glad you opened that PR :)).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another solution for targeting 3.4 would be to check the exception message to ensure the catch is legit.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do you think about this?

if (!$e instanceof UninitializedPropertyException
    // For versions without UninitializedPropertyException check the exception message
    && (\class_exists(UninitializedPropertyException::class) || false === \strpos($e->getMessage(), 'You should initialize it'))
) {
    throw $e;
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We would only need to check the message in 3.4 and then when merging branches up, use the type check in master instead. No need for more complexity here in your patch.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For 3.4 the type is never available with the current dependency constraint.
For 4.4, 5.0 and master it might be available.
I think we need to keep both checks in place until that dependency is bumped.

// Skip unitialized properties on $data
$this->catchUninitializedPropertyException($e);
}
} else {
$form->setData($config->getData());
}
Expand Down Expand Up @@ -76,16 +83,54 @@ public function mapFormsToData($forms, &$data)
$propertyValue = $form->getData();
// If the field is of type DateTimeInterface and the data is the same skip the update to
// keep the original object hash
if ($propertyValue instanceof \DateTimeInterface && $propertyValue == $this->propertyAccessor->getValue($data, $propertyPath)) {
if ($propertyValue instanceof \DateTimeInterface && $propertyValue == $this->getPropertyValue($data, $propertyPath)) {
continue;
}

// If the data is identical to the value in $data, we are
// dealing with a reference
if (!\is_object($data) || !$config->getByReference() || $propertyValue !== $this->propertyAccessor->getValue($data, $propertyPath)) {
if (!\is_object($data) || !$config->getByReference() || $propertyValue !== $this->getPropertyValue($data, $propertyPath)) {
$this->propertyAccessor->setValue($data, $propertyPath, $propertyValue);
}
}
}
}

/**
* Get the property value per PropertyAccessor.
* Treat uninitialized properties as null.
*
* @param object|array $objectOrArray The object or array to traverse
* @param string|PropertyPathInterface $propertyPath The property path to read
*
* @return mixed The value at the end of the property path
*
* @throws Exception\InvalidArgumentException If the property path is invalid
* @throws Exception\UnexpectedTypeException If a value within the path is neither object nor array
*/
private function getPropertyValue($data, $propertyPath)
{
try {
return $this->propertyAccessor->getValue($data, $propertyPath);
} catch (AccessException $e) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we apply the same logic as above to not blindly silence all exceptions that could be thrown here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think there should be any exception for failed reading of a property when trying to write it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there cannot be any exceptions, we shouldn't add the catch here to not hide any bugs IMO.

Copy link
Contributor Author

@ph-fritsche ph-fritsche Apr 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that the implementation tries to check for the current value before calling setValue.

class Foo1 {
    public int $bar;
}

One should be able to map a form with bar=123 to Foo1, but it will fail with UninitializedPropertyException.

class Foo2 {
    private $bar;
    public function setBaz($value) {
        $this->bar = $value * 2;
    }
}

One should be able to map a form with baz=123 to Foo2, but it will fail with a NoSuchPropertyException.

It is at least confusing and IMO a bug, that setting a value fails because reading it is not possible.

I decided against catching Throwable here, because e.g. RuntimeException thrown by PropertyAccessor points to bugs in the implementation with which one can not expect the consecutive setValue() to work.

Maybe @HeahDude can give further insight, but as I understood the current implementation the AccessException types point to errors in a specific read or write access, while the other exeptions point to wrong usage of PropertyAccessor.

Copy link
Member

@xabbuh xabbuh Apr 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not against ignoring errors caused by uninitialized properties. I just think that ignoring any other errors that might happen is not the desired solution. I suggest to update the new method like this:

private function getPropertyValue($data, $propertyPath)
{
    try {
        return $this->propertyAccessor->getValue($data, $propertyPath);
    } catch (AccessException $exception) {
        // Skip unitialized properties on $data
        // For versions without UninitializedPropertyException check the exception message
        if (!$e instanceof UninitializedPropertyException && (class_exists(UninitializedPropertyException::class) || false === strpos($e->getMessage(), 'You should initialize it'))) {
            throw $e;
        }
    }
}

We can then reuse this method in the setData() call above like this:

$form->setData($this->getPropertyValue($data, $propertyPath));

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But to move this foward: Will this get an approval, if I change and comment the code here and make the full request again for the master?

I don't know. I guess I would rather open an issue first and gather some feedback.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ph-fritsche Do you still like to finish the PR though?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, of course. I was just waiting for the feedback in #36754.

We can then reuse this method in the setData() call above like this:

$form->setData($this->getPropertyValue($data, $propertyPath));

That would replace the old bug by a new one as it sets data that is not there.

class Foo {
 public string $bar;
 public string $baz;
}

// this should be the same
$foo = new Foo();
$foo->bar = 'bar';
$propertyPathMapper->mapDataToForm($foo, [$someForm]);

// as this
$propertyPathMapper->mapDataToForm(['bar' => 'bar'], [$someForm]);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am afraid I do not understand. What exactly would be the new bug you are talking about?

As far as I can see #36754 would be a new feature affecting master only. So I do not really see how that would affect this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Mapper should transfer data from one representation to the other.
It should fail if it either can not do its job or if it can not guarantee a certain behavior.

The previous implementation introduced restrictions that are neither documented nor are intended. (They could not be, as the pivotal problem leading to this PR came through type hinted properties that didn't exist when the implementation was written.)

Now you could argue that supporting those new concepts is by itself a new feature - then one should close this PR and constrain the dependency to PHP<7.4.
If one supports those, one should do it right. And that means that

class Foo {
  // this
  public string $bar;
  // is different than this
  public string $baz = null;
}
// and this should never map null to someForm['bar']
$propertyPathMapper->mapDataToForm($foo, [$someForm]);

I committed another change that disables using mapFormToData() with setters when the getter is missing, as you considered this a new feature.
(Funny side note: mapDataToForm() works, if the setter is missing.)

// The following line might be removed in future versions
// See https://github.com/symfony/symfony/issues/36754
$this->catchUninitializedPropertyException($e);

return null;
}
}

/**
* Throw everything but UninitializedPropertyException.
*/
private function catchUninitializedPropertyException(AccessException $e)
{
if (!$e instanceof UninitializedPropertyException
// For versions without UninitializedPropertyException check the exception message
&& (class_exists(UninitializedPropertyException::class) || false === strpos($e->getMessage(), 'You should initialize it'))
) {
throw $e;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
use Symfony\Component\Form\Extension\Core\DataMapper\PropertyPathMapper;
use Symfony\Component\Form\Form;
use Symfony\Component\Form\FormConfigBuilder;
use Symfony\Component\Form\Tests\Fixtures\TypehintedPropertiesCar;
use Symfony\Component\PropertyAccess\PropertyAccess;
use Symfony\Component\PropertyAccess\PropertyAccessorInterface;
use Symfony\Component\PropertyAccess\PropertyPath;
Expand Down Expand Up @@ -113,6 +114,22 @@ public function testMapDataToFormsIgnoresUnmapped()
$this->assertNull($form->getData());
}

/**
* @requires PHP 7.4
*/
public function testMapDataToFormsIgnoresUninitializedProperties()
{
$engineForm = new Form(new FormConfigBuilder('engine', \stdClass::class, $this->dispatcher));
$colorForm = new Form(new FormConfigBuilder('color', \stdClass::class, $this->dispatcher));

$car = new TypehintedPropertiesCar();
$car->engine = new \stdClass();

$this->mapper->mapDataToForms($car, [$engineForm, $colorForm]);

$this->assertSame($car->engine, $engineForm->getData());
}

public function testMapDataToFormsSetsDefaultDataIfPassedDataIsNull()
{
$default = new \stdClass();
Expand Down Expand Up @@ -293,13 +310,29 @@ public function testMapFormsToDataIgnoresDisabled()
$config->setPropertyPath($propertyPath);
$config->setData($engine);
$config->setDisabled(true);
$form = new Form($config);
$form = new SubmittedForm($config);

$this->mapper->mapFormsToData([$form], $car);

$this->assertSame($initialEngine, $car->engine);
}

/**
* @requires PHP 7.4
*/
public function testMapFormsToUninitializedProperties()
{
$engine = new \stdClass();
$car = new TypehintedPropertiesCar();
$config = new FormConfigBuilder('engine', \stdClass::class, $this->dispatcher);
$config->setData($engine);
$form = new SubmittedForm($config);

$this->mapper->mapFormsToData([$form], $car);

$this->assertSame($engine, $car->engine);
}

/**
* @dataProvider provideDate
*/
Expand Down Expand Up @@ -339,7 +372,7 @@ public function isSubmitted()
}
}

class NotSynchronizedForm extends Form
class NotSynchronizedForm extends SubmittedForm
{
public function isSynchronized()
{
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
<?php
namespace Symfony\Component\Form\Tests\Fixtures;

class TypehintedPropertiesCar
{
public ?\stdClass $engine;
public ?\stdClass $color;
}
Morty Proxy This is a proxified and sanitized view of the page, visit original site.