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

I changed the target branch to 3.4.
I'm not sure if catchingUninitializedPropertyException 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?

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

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 :)).

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

How do you think about this?

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

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.

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.

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

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

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

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

class Foo1 {publicint$bar;}

One should be able to map a form withbar=123 toFoo1, but it will fail withUninitializedPropertyException.

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

One should be able to map a form withbaz=123 toFoo2, but it will fail with aNoSuchPropertyException.

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

I decided against catchingThrowable here, because e.g.RuntimeException thrown byPropertyAccessor points to bugs in the implementation with which one can not expect the consecutivesetValue() to work.

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

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:

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

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

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

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.

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

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

We can then reuse this method in thesetData() 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]);

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 affectingmaster only. So I do not really see how that would affect this PR.

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 toPHP<7.4.
If one supports those, one should do it right. And that means that

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

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