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

[Serializer] Allow to provide a "normalizer" callable to normalize a property during denormalization #27933

Open
ogizanagi opened this issue Jul 12, 2018 · 16 comments
Labels
Feature Keep open RFC RFC = Request For Comments (proposals about features that you want to be discussed) Serializer
Projects

Comments

@ogizanagi
Copy link
Member

ogizanagi commented Jul 12, 2018

Description

When dealing with DTOs & serializer and ObjectNormalizer, a missing feature to me is the ability to normalize values before actually setting the property in the DTOs. DTOs are often written with public properties only and writing custom setters (e.g: for trimming a string or change empty strings from the input to null) for it or a custom denormalizer is a bit overhead for the developer. The idea would be to be able to pass a normalizer callable in metadata that will be called by the ObjectNormalizer.

Another approach would be to rely on a PropertyAccessor annotation. #22190 inits the metadata support on this component, so it can fit as well.

Example

final class AddNotePayload
{
    /**
     * @var string
     *
     * @Assert\NotBlank()
     * @Serializer\Denormalizer("trim")
     */
    public $content;

    /**
     * @var \DateTime|null
     *
     * @Assert\Type(\DateTime::class)
     */
    public $createdAt;
}

or

final class AddNotePayload
{
    /**
     * @var string
     *
     * @Assert\NotBlank()
     * @PropertyAccessor(setterNormalizer="trim")
     */
    public $content;

    /**
     * @var \DateTime|null
     *
     * @Assert\Type(\DateTime::class)
     */
    public $createdAt;
}

Potential issues

When using AbstractObjectNormalizer::DISABLE_TYPE_ENFORCEMENT => true, we might get an unexpected type, thus the normalizer callable may fail due to incompatible type.

A solution might be to not execute the normalizer unless type is actually matching but then it probably has to live in the Serializer and not in the PropertyAccessor component.
Or the PropertyAccessor could catch any exception thrown by the normalizer callable and still set the value (could be configurable with a force option or whatever).

We might also provide a set of common normalizers that'll only act upon expected types and let the exception raise up if another user defined normalizer does not account properly for type/data (so native trim function usage would be discouraged in favor of a more clever PropertyAccessor\normalizers\trim() function).

In any case, proper validation must happen after and reject the non-matching value. So what we really want here is only trying to normalize a valid value (according to the type).

@ogizanagi ogizanagi added Serializer Feature RFC RFC = Request For Comments (proposals about features that you want to be discussed) labels Jul 12, 2018
@linaori
Copy link
Contributor

linaori commented Jul 12, 2018

This is almost what transformers do in forms... right?

@ogizanagi
Copy link
Member Author

Well, no. Form transformers does allow doing such, it's true. But in their design they're expected to perform bijective transformation. Normalization is not. Events are most suitable for this.

But actually how do you think it relates to this feature request? Symfony Form and Serializer components have different use-cases (even if some can be similar). We don't want to transform the Serializer component into Form component if that's what you fear.

@linaori
Copy link
Contributor

linaori commented Jul 12, 2018

I often use a transformer to transform an ID or similar to an object in my DTO, which seems to be something these annotation can handle in a cleaner way. I also often have to "clean" an input (like trim or removing all spaces, including in-between) for forms, which also seems to be a perfect use-case for the annotation you proposed.

Maybe I'm misunderstanding your feature request incorrectly though 😅

@ogizanagi
Copy link
Member Author

ogizanagi commented Jul 12, 2018

I also often have to "clean" an input (like trim or removing all spaces, including in-between)

Good news are if we choose the PropertyAccessor approach, you'll indeed benefit from this in forms too. But for the simple trim use-case, this is already supported natively by the Form component actually (http://symfony.com/doc/current/reference/forms/types/form.html#trim).

About transforming an ID/hash to an object (and other transformations), that's not what I target here.
That would also mean input type would mismatch expected type. Considering the potential issue mentioned in PR description, the possible solution would ignore the normalizer.
However, if implemented in the PropertyAccessor component, it would allow doing such for your own use-cases.

But again, transformations were not really what I intended in this feature request 😄
That's what (Serializer) normalizers are for, but we perhaps miss a @Serializer\Context() annotation allowing to fine-grain normalizer selection according to the use-case (e.g a normalizer transforming a hash to an object vs a normalizer denormalizing the same object properties) as expressed in #23932.

@carsonbot
Copy link

Thank you for this suggestion.
There has not been a lot of activity here for a while. Would you still like to see this feature?

@falkenhawk
Copy link

Yes.

@carsonbot carsonbot removed the Stalled label Dec 20, 2020
@julienfalque
Copy link
Contributor

I would say that both normalization (trimming, enforcing format, ...) and validation should be part of your domain's model, not in your integration with the framework, so it should happen in DTOs, Value Objects or whatever domain objects you use. Normalizing and validating in setters is not ideal though, but maybe #38487 will help by using constructors instead.

Regarding validation, I proposed #38472 which should help throwing exceptions in your domain objects and have the serializer present validation errors like the Validation component does.

@carsonbot
Copy link

Thank you for this suggestion.
There has not been a lot of activity here for a while. Would you still like to see this feature?

@falkenhawk
Copy link

I believe it's been covered by #39399 ?

@carsonbot carsonbot removed the Stalled label Jun 21, 2021
@ogizanagi
Copy link
Member Author

@falkenhawk : No, this PR was about allowing to provide a Serializer context per-property basis (so is about context and metadata).

This issue is about a new feature that'll allow to "normalize" / clean the raw data before being denormalized to an object. Main motivation was trimming strings, as done by the Form component on text inputs for instance, but could be extended to other treatments (though I don't have much in mind right now, appart from transforming an empty string to null perhaps). Hence I'm not sure these two suggestions are the way to go, but I had such use-cases in multiple previous projects before.

@hafkenscheid
Copy link

I like this solution. Are there any plans to start working on this?

@ogizanagi
Copy link
Member Author

ogizanagi commented Jun 30, 2021

Regarding the issues & feature requests that echo to this one and what I encountered in my own projects, I think the use-cases are too narrowed for the PropertyAccess proposition to justify a whole new metadata system in it to handle annotations/attributes. But there is still #38515 which is opened on this regard.

About implementing it in the Serializer, #39399 might have significantly simplify the approach: we'll don't truly need a new @Serializer\Denormalizer("trim") annotation/attribute and a new entry in the metadata system, but could simply add a new context key in the ObjectNormalizer, so we'll could do something like:

final class AddNotePayload
{
    /**
     * @var string
     *
     * @Serializer\Context({ ObjectNormalizer::FIELD_DENORMALIZER = 'trim' })
     */
    public $content;
}

This key would be checked for each field during denormalization and use the normalizer function if present in the context.

For better DX, providing a simpler annotation/attribute would be as simple as extending the Context class (see #39399 (comment) however, to reconsider), so we'll use:

final class AddNotePayload
{
    /**
     * @var string
     * 
     * @Serializer\FieldDenormalizer('trim')
     */
    public $content;
}

Regarding the two most identified use-cases, i.e:

  • trimming a string
  • casting an empty string to null
  • casting a numeric-like string to float

we could provide these normalizer functions, with proper type/data check, in the component, so these are easy to reference:

final class AddNotePayload
{
    /**
     * @var string
     *
     * @Serializer\FieldDenormalizer('Serializer\normalizers\trim')
     */
    public $content;
}

and in the same way, annotations/attributes shortcuts through extending the original Context class:

final class AddNotePayload
{
    /**
     * @var string
     *
     * @Serializer\Trim()
     */
    public $content;
}

// or

final class AddNotePayload
{
    /**
     * @var string|null
     *
     * @Serializer\NullIfEmpty()
     */
    public $content;
}

@carsonbot
Copy link

Thank you for this suggestion.
There has not been a lot of activity here for a while. Would you still like to see this feature?

@palansher
Copy link

Regarding the two most identified use-cases, i.e:

trimming a string
casting an empty string to null

My use-cases also include first char capitalization for human names..

@andrew-belac
Copy link

Does anyone have a workaround or solution for this. If we don't it means we either have to remove readonly from our DTO classes or create a new DTO class with the data mapped. For us it needs to be done on creation since this DTO will be sent asynchronously to many subsribers.

@longshadowdev
Copy link

Does anyone have a workaround or solution for this. If we don't it means we either have to remove readonly from our DTO classes or create a new DTO class with the data mapped. For us it needs to be done on creation since this DTO will be sent asynchronously to many subsribers.

Not sure how 'official' you want the workaround to be, but this is what I have been doing. I basically use a trait on the class being populated.

trait SetterTrait
{
    public function __set($name, $value): void 
    {
        $value = trim($value);

        // Don't set empty values. Fields should be nullable and/or have
        // default values set instead.
        if (empty($value)) {
            return;
        }

        // Disallow dynamic properties. A property should exist if we are to
        // populate it. This future proofs for php 8.3 as well
        if (property_exists($this, $name)) {
            $this->{$name} = $value;
        }
    }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Keep open RFC RFC = Request For Comments (proposals about features that you want to be discussed) Serializer
Projects
Development

No branches or pull requests

9 participants