-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[ObjectMapper] Object to Object mapper component #51741
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
Conversation
Hey! To help keep things organized, we don't allow "Draft" pull requests. Could you please click the "ready for review" button or close this PR and open a new one when you are done? Note that a pull request does not have to be "perfect" or "ready for merge" when you first open it. We just want it to be ready for a first review. Cheers! Carsonbot |
0be33de
to
cf9d105
Compare
cf9d105
to
139a158
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition to the API Platform usecase, this would help with people using form DTO's... which is a cool idea, but kind of a pain in practice due to the mapping.
So, I'm very interested in this. If done well, it would enable us to more strongly recommend the DTO pattern without sacrificing DX (due to all the boring mapping you normally do)
63322b3
to
3488fd9
Compare
Sorry guys, noob question here, but I was not at the conf so I didn't get the rationale. I see the point for APIP or such solutions, but I can't see why it should be a Symfony component, as I don't see the value it can provide compared to a basic custom mapper. And I don't see why we would want to get rid of custom mappers in generic projects if there's no gain except "one less class in Other than that, once again, the DX is awesome, maybe except one or two small details. |
That's a fair question. In practice, in a rich DTO system, when I have objects with relationships, things often get more complex than one class for the mapping. For example, I'm mapping Also, even if the solution is simple, there is some advantage to having a core interface & system for mapping. For API Platform, it opens up the possibility of mapping a DTO with |
@weaverryan I also really liked your examples with the User DTOs (mapping password etc.) which are quite out of the scope of API Platform but are really useful within Symfony or with custom controllers altogether. Maybe we can share these? Actually it's been quite some time that many Symfony users think that an AutoMapper component inside Symfony would be quite useful. I started a prototype after discussing with Ryan and therefore wanted to propose this as a component to help our community members. |
Sure :). Whether it's API Platform or using DTOs with forms, here are some requirements I had for a mapper: A) |
First, thanks for your time!
I do have the same "problem" on a regular basis. In fact, pretty much every time I give a Symfony training (like twice a month at least), since it's pretty much part of the application we build as an exercice for the trainings at SL. And again, I don't deny the DX gain on this, maybe it needs a bit more work (like, does it work when mapping collections, or nested collections?). But maybe I'm wrong, but I can't see something relying this much on Reflection being as effective as the boring solution, even with Mappers for nested properties. If you tell me the accepted tradeof is perf vs DX, why not, but I'm not sure it'll be used that much then, and so I'm back to my first question.
Again, no question how it could be useful for APIP. But in that case IMHO it should stay in APIP. Basically, if the perf question is resolved, and the gain is substantial not only for DX, I'm all for it. Otherwise, I'm not sure. |
AutoMapper is a subject we hold close to heart here at JoliCode ❤️ (remember #30248? 4 years old already) and I would like to highlight another approach to the one exposed here. I worked on bringing AutoMapper, previously in the JanePHP organization, now in JoliCode's in Symfony as a standalone component: Korbeil#1 It has not been opened on the Symfony repository yet as an excess of precaution and a will to bring something pre-approved (#30248 was a ton of work and never got merged so we were careful not to make the same mistake). It has been discussed on Symfony's Slack with some core maintainers but that didn't bring real feedback nor reviews. If we step back for a sec, we all know that the Serializer revamp will not be merged before 7.1 at best. Beside, the current AutoMapper implementation looks quick and simple as it allows to map only DTO together. Meanwhile, we could take more time to consider my PR and deliver a complete and performant AutoMapper with the new Serializer implementation where the normalization part could leverage this AutoMapper and bring even more speed to the new Serializer #51718. Performance should always be taken into account as a primary feature - we use Symfony on huge and heavy workload applications and it's not ok to have some features doing runtime Reflection with no cache. Code using such components would need to be hardly challenged from code reviews. And that’s why we do think AST is a very powerful way to get this performance.
Could you elaborate on this statement? While your implementation is really nice and easy to read / maintain, from an user perspective, jolicode/automapper is as simple and accessible: $automapper->map(source, target); We all must keep in mind the benefit for Symfony's users, no matter what is our company, or the code ownership. Currently, we have 2 different suggestions for a new AutoMapper component, let's work together to leverage the best of both and bring something useful to the users. |
This comment was marked as duplicate.
This comment was marked as duplicate.
I tried using the janephp/automapper (maybe the jolicode one has improved?) but I had a very hard time with A, B, D & E from this list - #51741 (comment) So yes, the interface is simple. I found customizing my mapping to be really hard / impossible in some cases. But these are things that can be improved :). I am more interested that there IS a solution that exists, not which one... though I'd love to collaborate on the DX for whatever is decided. |
Probably a minor point, but i'd suggest to drop the "Auto" part of the component, and maybe keep it for the implementation.
For me if Symfony provide such component, i expect it to have a basic interface, hiding all the complexity / configuration behind.... including the possibility to use plain manual PHP / service code in implementations. So maybe just name it namespace Symfony/Component/Mapper;
class Automapper implements MapperInterface (ObectMapper does not provide much more, and impede potential evolutions) |
After some discussions I:
I've kept my old implementation here in case we want to track evolution. The description and title will be updated shortly. Old implementation``` use Symfony\Component\AutoMapper\AutoMapper;use Symfony\Component\AutoMapper\Attributes\MapIf; use Symfony\Component\AutoMapper\Attributes\MapTo; use Symfony\Component\AutoMapper\Attributes\MapWith; // This maps class
} $mapper = new AutoMapper();
|
Hi, I have some suggestions. I think that in the attributes, the use of "Map" is redundant (MapTo, MapWith, MapIf, etc..). I think you can drop the "Map" prefix Also, I suggest to drop the MapWith, and change the "To" attribute to be able to "transform" a value before applying it :
|
cool idea!
|
|
If I understood it well, by design, a class can be mapped only to one another class ? Isn't it a very drastic limitation ? I could want to map an entity to more than one kind of DTO, for example:
Is this achievable with the code in its current state ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
6 months to fine-tune and 6 more to experiment irl
4ad6548
to
ee21586
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here are some suggestions
src/Symfony/Component/ObjectMapper/ConditionCallableInterface.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/ObjectMapper/Metadata/ReflectionObjectMapperMetadataFactory.php
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM thanks. Please squash + rebase while fixing my comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With minor comments 🙂
@nicolas-grekas rebased |
src/Symfony/Component/ObjectMapper/.github/workflows/check-subtree-split.yml
Show resolved
Hide resolved
ceb4a5a
to
b2acce8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some random/minor comments while reading the code.
Thank you @soyuka. |
Congrats 🎉 |
…t (soyuka) This PR was squashed before being merged into the 7.3 branch. Discussion ---------- [ObjectMapper] Object to Object mapper component | Q | A | ------------- | --- | Branch? | 7.3 | Bug fix? | no | New feature? | yes | Deprecations? | no | License | MIT | Doc PR | TBD this description will be used as docs ## Why ? In the train back from [API Platform Con](https://api-platform.com/con/2023/) and after watching `@weaverryan` conference about API Platform's feature that helps [separating Doctrine Entities from POPOs](https://symfonycasts.com/screencast/api-platform-extending/state-options), we had the feeling that such a component is definitely missing from the Symfony ecosystem. Current implementations (automapper-plus, janephp/automapper) are good but feel to complicated when you want something accessible (DX and complexity). Here we're not trying to have exceptional performance, we just want an accessible API with few mandatory features. Basically those are: - map properties from a class/object to another class/object (`Map(target: A::class)`) - configure the property name each property is mapped to (`Map(target: 'prop')`) - change the value during transformation (`Map(transform: 'ucfirst')`) - control whether the value should be mapped or not (`Map(if: 'boolval')`) - reuse objects if possible (works better with doctrine UOW) Other implementation details: - if a property exists in B, it'll "automatically" be mapped (doesn't handle type errors, should we ?) - if a property doesn't exists in B it'll "automatically" **not** be mapped The rest is not "auto". ## Mapper Component The Mapper component allows you to map an object to another object, facilitating the mapping using attributes. ### Usage ```php use Symfony\Component\ObjectMapper\ObjectMapper; use Symfony\Component\ObjectMapper\Attributes\Map; // This maps class `A` to class `B`. #[Map(B::class)] class A { // This maps A::foo to B::bar. #[Map(target: 'bar')] public string $foo; // This calls ucfirst on A::transform #[Map(transform: 'ucfirst')] public string $name; // This doesn't map A::bar if it's value is falsy. #[Map(if: 'boolval')] public bool $bar = false; } $mapper = new ObjectMapper(); $mapper->map(new A); ``` The `Map` attribute has the following signature: ```php #[Attribute(Attribute::TARGET_CLASS | Attribute::TARGET_PROPERTY | Attribute::IS_REPEATABLE)] final class Map { /** * * `@param` null|string|class-string $source The property or the class to map to * `@param` null|string|class-string $target The property or the class to map from * `@param` string|callable(mixed $value, object $object): bool $if A Symfony service name or a callable that instructs whether to map * `@param` CallableType|CallableType[] $transform A Symfony service name or a callable that transform the value during mapping */ public function __construct( public readonly ?string $source = null, public readonly ?string $target = null, public readonly mixed $if = null, public readonly mixed $transform = null ){ } } ``` `if` and `transform` are callable or Symfony services, not that we need to introduce an interface for services to implement, this will follow if we want to introduce the component inside the Framework Bundle. The mapper also takes a `$source` argument if one needs to update an object instead of creating a new one: ```php $b = new B; $mapper = new ObjectMapper(); $mapper->map(new A, $b); ``` ## TODO - [x] AutoMapper or Automapper or something else? `Mapper` component with an `AutoMapper` implementation. - [x] max depth? Supports recursivity, `maxDepth` is for serializers. Commits ------- 3f99b03 [ObjectMapper] Object to Object mapper component
* | ||
* @return list<Mapping> | ||
*/ | ||
public function create(object $object, ?string $property = null, array $context = []): array; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@soyuka Shouldn't a @throws MappingException
be added to this interface too since ReflectionObjectMapperMetadataFactory are throwing some ? I can do the PR if you agree with
…(soyuka) This PR was merged into the 7.3 branch. Discussion ---------- [FrameworkBundle] Binding for Object Mapper component | Q | A | ------------- | --- | Branch? | 7.3 | Bug fix? | no | New feature? | yes | Deprecations? | no | Issues | Fix #51741 (comment) | License | MIT TODO: Update UPGRADE-*.md and src/**/CHANGELOG.md files Needs: #51741 please check only the last commit as I didn't yet squashed the commits of the Component. Commits ------- 133d2b0 [FrameworkBundle] Object Mapper component bindings
…(soyuka) This PR was merged into the 7.3 branch. Discussion ---------- [FrameworkBundle] Binding for Object Mapper component | Q | A | ------------- | --- | Branch? | 7.3 | Bug fix? | no | New feature? | yes | Deprecations? | no | Issues | Fix symfony/symfony#51741 (comment) | License | MIT TODO: Update UPGRADE-*.md and src/**/CHANGELOG.md files Needs: symfony/symfony#51741 please check only the last commit as I didn't yet squashed the commits of the Component. Commits ------- 133d2b09730 [FrameworkBundle] Object Mapper component bindings
Why ?
In the train back from API Platform Con and after watching @weaverryan conference about API Platform's feature that helps separating Doctrine Entities from POPOs, we had the feeling that such a component is definitely missing from the Symfony ecosystem. Current implementations (automapper-plus, janephp/automapper) are good but feel to complicated when you want something accessible (DX and complexity). Here we're not trying to have exceptional performance, we just want an accessible API with few mandatory features. Basically those are:
Map(target: A::class)
)Map(target: 'prop')
)Map(transform: 'ucfirst')
)Map(if: 'boolval')
)Other implementation details:
The rest is not "auto".
Mapper Component
The Mapper component allows you to map an object to another object,
facilitating the mapping using attributes.
Usage
The
Map
attribute has the following signature:if
andtransform
are callable or Symfony services, not that we need to introduce an interface for services to implement, this will follow if we want to introduce the component inside the Framework Bundle.The mapper also takes a
$source
argument if one needs to update an object instead of creating a new one:TODO
Mapper
component with anAutoMapper
implementation.maxDepth
is for serializers.