Skip to content
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

DX initiative #20

Closed
4 of 6 tasks
Korbeil opened this issue Dec 9, 2023 · 4 comments
Closed
4 of 6 tasks

DX initiative #20

Korbeil opened this issue Dec 9, 2023 · 4 comments
Labels
enhancement New feature or request

Comments

@Korbeil
Copy link
Member

Korbeil commented Dec 9, 2023

Hey,

The objective is to make AutoMapper easier to work with for developpers.
In this issue I'll try to list all stuff developpers needs to fit their needs when using AutoMapper.

All the following stuff are examples and subject to change in their implementation.

  • Being able to give a custom property output for a given target
class Foo
{
  public function __construct(
    #[MapTo(('array', 'phrase')]
    #[MapTo(Bar::class, 'phrase')]
    public string $sentence,
  ) {
  }
}
  • Being able to give a custom property input for a given target
class Foo
{
  public function __construct(
    #[MapFrom(('array', 'phrase')]
    #[MapFrom(Bar::class, 'phrase')]
    public string $sentence,
  ) {
  }
}
  • Being able to map a field only if a certain condition is filled for a given target
class Foo
{
  public function __construct(
    #[MapIf(('array', fn() => true)]
    #[MapIf(Bar::class, fn() => false)]
    public string $sentence,
  ) {
  }
}
  • Having a way to override / custom the mapping of a property for a given transformation
final readonly class FromSourceCustomPropertyTransformer implements CustomPropertyTransformerInterface
{
    public function supports(string $source, string $target, string $propertyName): bool
    {
        return $source === UserDTO::class && $target === 'array' && $propertyName === 'name';
    }

    public function transform(mixed $input): mixed
    {
        return "{$input} set by custom property transformer";
    }
}
  • Having a way to override / custom the mapping for a given transformation as a whole
final readonly class FromTargetCustomModelTransformer implements CustomModelTransformerInterface
{
    public function supports(string $source, string $target): bool
    {
        return $source === 'array' && $target === AddressDTO::class;
    }

    public function transform(mixed $input): mixed
    {
        $addressDTO = new \AutoMapper\Tests\Fixtures\AddressDTO();
        $addressDTO->city = "{$input['city']} from custom model transformer";

        return $addressDTO;
    }
}
  • Having a way to add custom code during some steps of the transformation:
    • beforeInitialization
    • afterInitialization
    • afterHydratation
class Foo
{
  public function __construct(
    public string $sentence,
  ) {
  }

  #[MapEvent(Event::AFTER_INITIALIZATION)]
  public function doctrineHook(mixed $object): void
  {
    // link my entity to Doctrine UOW
  }
}
@Korbeil Korbeil pinned this issue Dec 9, 2023
@nikophil
Copy link
Collaborator

nikophil commented Dec 11, 2023

hi!

  • about MapTo / MapFrom, I'm wondering if I wouldn't have chosen to pass an associative array:
MapTo(['array' => 'phrase', Bar::class => 'phrase'])
  • about MapIf, be careful we cannot pass an arrow function in an attribute (but you can pass a static callable such as [self::class, 'method])

  • an event system could be nice, but I'm not sure to understand the example mentioning doctrine? what could be done with this?

  • I'm wondering if for CustomModelTransformerInterface we should not introduce a concept like NormalizerAware in order to only fill SOME fields a let the rest be filled automatically

@nikophil
Copy link
Collaborator

if I recall Ryan's message:

A) User.plainPassword -> maps to -> User.password but we need to go through the password hasher
B) Same as above, but only map if User.plainPassword is not null (don’t try to hash an empty string and override the existing hashed password)
C) Mapping A -> B, but B has some extra property on it that depends on, let’s say… 3 different properties on A and needs a service to calculate the final value.
D) Mapping from UserDto -> User and because this is a PATCH operation / edit page, UserDto.id=5 and so we should actually query the database for User where id=5 instead of creating a new instance.
E) Same as above, but with a relation: ProductDto.category is a CategoryDto and ProductDto.category.id=9. So when mapping ProductDto.category -> Product.category, the Category entity will be loaded from somewhere else (e..g database) but not instantiated.

A) I think this could be done with a CustomPropertyTransformerInterface as a service (we'd need DI to access PasswordHasher)
But may we introduce some nicer syntax?
transformer could be:

  • an object: trasnformer: new MyPasswordEncoder() is a valid syntax
  • a string, and we'd grab a service from the DI

if method is omitted, the transformer needs to be callable.
We could even be possible to pass transform: [self::class, 'transformer'] to keep the transformation in the same class.

class UserDto
{
  public function __construct(
    #[MapTo(target: UserEntity::class, field; 'password', transformer: MyPasswordEncoder::class, method: 'encodePassword')]
    public string $plainPassword,
  ) {
  }
}

B) This makes me think that the MapIf() you mentioned does not clearly the "direction" of the mapping:
does #[MapIf(Bar::class, fn() => false)] is a condition from Bar::class or to Bar::class?
couldn't we add a condition property to #[MapTo] and #[MapFrom]? this would remove the ambiguity

C) I think the C could leverage flattening. We'd still need to think about a nice syntax for this.
I'm wondering if the C is not complex enough to use CustomModelTransformerInterface (he says we need a service to compute a field from three other ones)

I think D&E are more related to Api-Platform

@joelwurtz joelwurtz added the enhancement New feature or request label Mar 23, 2024
@joelwurtz
Copy link
Member

Most of comments were addressed, only remaining point is if we want an user to fully replace a Mapper for a specific source and target, i'm not sure since you can tell how to instantiate (with provider) and which property to map (with attributes and transformers) this roughly covers most use cases.

We can close this if we don't want it for the moment (i think there is already enough features for a 9.0 not need to add more)

@Korbeil
Copy link
Member Author

Korbeil commented Mar 31, 2024

Thanks a lot for all the work @joelwurtz <3 much appreciated, I think we can close this issue now.

@Korbeil Korbeil closed this as completed Mar 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants