[ObjectMapper] Map a collection of objects #60615
Conversation
makomweb
changed the title
There was a problem hiding this comment.
Because this is a new feature, don't forget to add a line in the changelog of the component! Also, don't forget to check fabbot which will provide some automated feedback on the code style.
| } | ||
| } | ||
|
|
||
| if (!empty($exceptions)) { |
There was a problem hiding this comment.
We avoid using empty() in Symfony, this may be replaced by:
| if (!empty($exceptions)) { | |
| if ($exceptions) { |
| interface MapMultipleInterface | ||
| { | ||
| /** | ||
| * @template T of object |
There was a problem hiding this comment.
Should this annotation be on the class instead of the method, so devs can use @implements MapMultipleInterface<Something>? I'm not sure this works well if the template definition is on the method
There was a problem hiding this comment.
@alexandre-daubois I suggest to keep it consistent with the ObjectMapperInterface in the same package. The annotation is on the method there too. What do you think?
|
|
||
| self::assertInstanceOf(D::class, $target[0]); | ||
| $firstTarget = $target[0]; | ||
| assert($firstTarget instanceof D); |
There was a problem hiding this comment.
We don't use assert(), also the instance is already checked above with self::assertInstanceOf(D::class, $target[0]);
There was a problem hiding this comment.
I only did this to support code completion of the IDE. Removed it.
| self::assertInstanceOf(D::class, $target[1]); | ||
| $secondTarget = $target[1]; | ||
| assert($secondTarget instanceof D); |
| self::assertEquals('foo2', $secondTarget->baz); | ||
| self::assertEquals('bar2', $secondTarget->bat); |
There was a problem hiding this comment.
We use self::assertSame wherever possible 🙂
|
|
||
| self::fail('Mapping of second source element should have thrown!'); | ||
| } catch (MapMultipleAggregateException $ex) { | ||
| self::assertEquals('Mapping source collection has failed.', $ex->getMessage()); |
There was a problem hiding this comment.
| self::assertEquals('Mapping source collection has failed.', $ex->getMessage()); | |
| self::assertSame('Mapping source collection has failed.', $ex->getMessage()); |
(Same for the occurrences below)
| self::assertCount(1, $target, 'Mapping first source collection object should have passed!'); | ||
| self::assertInstanceOf(D::class, $target[0]); | ||
| $firstTarget = $target[0]; | ||
| assert($firstTarget instanceof D); |
There was a problem hiding this comment.
Same as above, this is needed 🙂
makomweb
force-pushed
the
feat-60518/map-multiple-objects
branch
from
c658ef1 to
467fce5
Compare
|
@alexandre-daubois Thank you for your feedback. |
|
Oh I missed this. We could use #60442 for embedded collections, but I really don't understand the need for such a functionality inside the Mapper. About naming instead of MapMultiple I'd suggest What if your sources are of multiple types? What if you want to target different classes? What if you want to throw an exception after the first element fails to map? I'm concerned that adding collection mapping to the new ObjectMapper component might not be the best path forward at this stage. In my opinion, it risks blurring the component's focus. One could argue this goes against the Single Responsibility Principle, as the component's core job is transforming a single object, not handling iteration logic. I also feel that a simple foreach loop is often a clearer and more explicit solution for developers. For now, I think it's preferable to encourage that approach, as it avoids adding underlying complexity to a component that would benefit from staying lean and focused while it's still experimental. |
Add functionality to map multiple source objects to the specified target type.
This is my 1st PR to Symfony. Looking forward to all kinds of feedback.