[ObjectMapper] Add MappingException, MapCollection and MapTree attributes #60432
Conversation
|
Hey! I see that this is your first PR. That is great! Welcome! Symfony has a contribution guide which I suggest you to read. In short:
Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change. When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor! I am going to sit back now and wait for the reviews. Cheers! Carsonbot |
There was a problem hiding this comment.
Thanks for your interest in improving the object mapper. Could you maybe explain your use cases at #54476 and then we can work or propose ways to improve the mapper for these?
This PR is quite a draft, many ideas need to be discussed first. I like the MappingException changes but I don't see them being tested nor used in the actual code, therefore I'm not sure what to expect?
| * @author Devoton <oton.traore@email.com> | ||
| */ | ||
| #[Attribute(Attribute::TARGET_PROPERTY)] | ||
| class MapCollection |
There was a problem hiding this comment.
I'm 👎 for this as I don't really see the point, could you give me a real life example where you'd want something like this?
If we start supporting arrays in the ObjectMapper it can become quite complex, we should instead use a CollectionMapper but I'm quite not sure of the functional benefits behind it.
| public string $of, | ||
| public string $childrenProperty = 'children' | ||
| ) {} | ||
| } |
There was a problem hiding this comment.
I'm not sure about this, could you show the use case? The ObjectMapper alread supports recursive structures.
| @@ -25,6 +27,7 @@ | ||
| * @experimental | ||
| * | ||
| * @author Antoine Bluchet <soyuka@gmail.com> | ||
| * @author Devoton <oton.traore@gmail.com> |
There was a problem hiding this comment.
I don't think we use multiple author tags do we?
| @@ -55,11 +58,11 @@ public function map(object $source, object|string|null $target = null): object | ||
| $mappingToObject = \is_object($target); | ||
|
|
||
| if (!$target) { | ||
| throw new MappingException(\sprintf('Mapping target not found for source "%s".', get_debug_type($source))); | ||
| throw new MappingException(\sprintf('Mapping target not found for source \"%s\".', get_debug_type($source))); |
| $mappedTree[] = $mappedNode; | ||
| } | ||
|
|
||
| return $mappedTree; |
There was a problem hiding this comment.
I don't think its a good idea to include these functionalities directly in the ObjectMapper, we should instead create new kinds of Mappers that compose with the ObjectMapper
| $this->assertEquals($c->foo, 'donotmap'); | ||
| $this->assertEquals($c->doesNotExistInTargetB, 'foo'); | ||
| } | ||
| } |
| { | ||
| public static function forProperty(string $sourcePath, string $targetPath, string $expected, string $actual, $value = null): self |
There was a problem hiding this comment.
this is interesting but not used anywhere?
| */ | ||
| class MappingException extends RuntimeException | ||
| class MappingException extends \RuntimeException |
| use Symfony\Component\ObjectMapper\ObjectMapper; | ||
| use Symfony\Component\ObjectMapper\Attribute\MapCollection; | ||
|
|
||
| class MapCollectionTest extends TestCase |
There was a problem hiding this comment.
I think that this is wrong as the file name is not the same?
What does this PR do?
This PR introduces three new features to the ObjectMapper component:
Why is this needed?
These features improve error reporting and allow more complex object graph mapping, enhancing the flexibility and robustness of the ObjectMapper component.
How to test?
tests/directory../vendor/bin/phpunit tests/Additional notes
Please review the implementation and let me know if any changes are required.
Thank you for your time and feedback!