Skip to content

[ObjectMapper] Map a collection of objects #60615

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

Open
wants to merge 1 commit into
base: 7.4
Choose a base branch
from

Conversation

makomweb
Copy link

@makomweb makomweb commented Jun 1, 2025

Q A
Branch? 7.4
Bug fix? no
New feature? yes
Deprecations? no
Issues Fix #60518
License MIT

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.

@carsonbot carsonbot added this to the 7.4 milestone Jun 1, 2025
@makomweb makomweb changed the title [ObjectMapper] map a collection of objects [ObjectMapper] Map a collection of objects Jun 1, 2025
Copy link
Member

@alexandre-daubois alexandre-daubois left a comment

Choose a reason for hiding this comment

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

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)) {
Copy link
Member

Choose a reason for hiding this comment

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

We avoid using empty() in Symfony, this may be replaced by:

Suggested change
if (!empty($exceptions)) {
if ($exceptions) {

interface MapMultipleInterface
{
/**
* @template T of object
Copy link
Member

Choose a reason for hiding this comment

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

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


self::assertInstanceOf(D::class, $target[0]);
$firstTarget = $target[0];
assert($firstTarget instanceof D);
Copy link
Member

Choose a reason for hiding this comment

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

We don't use assert(), also the instance is already checked above with self::assertInstanceOf(D::class, $target[0]);

Comment on lines +40 to +42
self::assertInstanceOf(D::class, $target[1]);
$secondTarget = $target[1];
assert($secondTarget instanceof D);
Copy link
Member

Choose a reason for hiding this comment

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

Same here

Comment on lines +43 to +44
self::assertEquals('foo2', $secondTarget->baz);
self::assertEquals('bar2', $secondTarget->bat);
Copy link
Member

Choose a reason for hiding this comment

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

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());
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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);
Copy link
Member

Choose a reason for hiding this comment

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

Same as above, this is needed 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ObjectMapper] Add possibility to map multiple objects at once
3 participants