-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
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
Changes from all commits
252dffc
0575980
6c3aaf2
39b4abe
19679dc
a975d5e
db718c0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
||
|
@@ -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) { | ||
// Skip unitialized properties on $data | ||
$this->catchUninitializedPropertyException($e); | ||
} | ||
} else { | ||
$form->setData($config->getData()); | ||
} | ||
|
@@ -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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If there cannot be any exceptions, we shouldn't add the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 class Foo2 {
private $bar;
public function setBaz($value) {
$this->bar = $value * 2;
}
} One should be able to map a form with It is at least confusing and IMO a bug, that setting a value fails because reading it is not possible. I decided against catching Maybe @HeahDude can give further insight, but as I understood the current implementation the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 $form->setData($this->getPropertyValue($data, $propertyPath)); There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I don't know. I guess I would rather open an issue first and gather some feedback. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ph-fritsche Do you still like to finish the PR though? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, of course. I was just waiting for the feedback in #36754.
That would replace the old bug by a new one as it sets data that is not there.
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The Mapper should transfer data from one representation to the other. 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 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 |
||
// 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 |
---|---|---|
@@ -0,0 +1,8 @@ | ||
<?php | ||
namespace Symfony\Component\Form\Tests\Fixtures; | ||
|
||
class TypehintedPropertiesCar | ||
{ | ||
public ?\stdClass $engine; | ||
public ?\stdClass $color; | ||
} |
There was a problem hiding this comment.
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
.There was a problem hiding this comment.
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 broaderAccessException
here. Is there a scenario in which catching the other possible AccessExceptions is harmful in the light of a flexible implementation?There was a problem hiding this comment.
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.There was a problem hiding this comment.
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 :)).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.