Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[ObjectMapper] Add MappingException, MapCollection and MapTree attributes#60432

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Closed

Conversation

ghost
Copy link

@ghostghost commentedMay 16, 2025
edited by ghost
Loading

What does this PR do?

This PR introduces three new features to the ObjectMapper component:

  • MappingException: a detailed exception class for mapping errors.
  • MapCollection: an attribute that allows mapping arrays into collections of typed objects.
  • MapTree: an attribute to map recursive tree structures with children nodes.

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?

  • All new features are covered by unit tests in thetests/ directory.
  • Run tests locally with:./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!

loverg-c reacted with thumbs up emoji
@carsonbotcarsonbot added this to the7.3 milestoneMay 16, 2025
@carsonbot
Copy link

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has acontribution guide which I suggest you to read.

In short:

  • Always add tests
  • Keep backward compatibility (seehttps://symfony.com/bc).
  • Bug fixes must be submitted against the lowest maintained branch where they apply (seehttps://symfony.com/releases)
  • Features and deprecations must be submitted against the 7.3 branch.

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!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

@carsonbotcarsonbot changed the titlefeat(object-mapper): add MappingException, Mafeat(object-mapper): add MappingException, MapCollection and MapTree supportpCollection and MapTree …[ObjectMapper] feat(object-mapper): add MappingException, Mafeat(object-mapper): add MappingException, MapCollection and MapTree supportpCollection and MapTree …May 16, 2025
@nicolas-grekasnicolas-grekas changed the title[ObjectMapper] feat(object-mapper): add MappingException, Mafeat(object-mapper): add MappingException, MapCollection and MapTree supportpCollection and MapTree …[ObjectMapper] Add MappingException, MapCollection and MapTree attributesMay 16, 2025
@nicolas-grekas
Copy link
Member

nicolas-grekas commentedMay 16, 2025
edited
Loading

/cc@soyuka@Korbeil I guess

@devoton thanks for the PR. Can you please update the PR description to put back the table that was in the original PR template (and fill it in properly)? It's required for proper license attribution.

Copy link
Contributor

@soyukasoyuka left a comment

Choose a reason for hiding this comment

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

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 theMappingException 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
Copy link
Contributor

Choose a reason for hiding this comment

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

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'
) {}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure about this, could you show the use case? The ObjectMapper alread supports recursive structures.

@@ -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)));
Copy link
Contributor

Choose a reason for hiding this comment

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

why the\?

Choose a reason for hiding this comment

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

fabbot's suggestion, but this is a false-positive, they should all be removed please @devoton

$mappedTree[] = $mappedNode;
}

return $mappedTree;
Copy link
Contributor

Choose a reason for hiding this comment

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

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');
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

why the removal of this?

{
public static function forProperty(string $sourcePath, string $targetPath, string $expected, string $actual, $value = null): self
Copy link
Contributor

Choose a reason for hiding this comment

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

this is interesting but not used anywhere?

*/
class MappingException extends RuntimeException
class MappingException extends\RuntimeException
Copy link
Contributor

Choose a reason for hiding this comment

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

why the change?

use Symfony\Component\ObjectMapper\ObjectMapper;
use Symfony\Component\ObjectMapper\Attribute\MapCollection;

class MapCollectionTest extends TestCase
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that this is wrong as the file name is not the same?

@ghostghost closed this by deleting the head repositoryMay 21, 2025
This pull request wasclosed.
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Projects
None yet
Milestone
7.3
Development

Successfully merging this pull request may close these issues.

4 participants
@carsonbot@nicolas-grekas@soyuka@otontraore

[8]ページ先頭

©2009-2025 Movatter.jp