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] Support for denormalizing empty strings to null #41890

Closed
hafkenscheid opened this issue Jun 28, 2021 · 3 comments
Closed

[Serializer] Support for denormalizing empty strings to null #41890

hafkenscheid opened this issue Jun 28, 2021 · 3 comments

Comments

@hafkenscheid
Copy link

Description
I receive XML objects that have empty tags, which in our application should be null.
However, the serializer serializes these properties as an empty string (""). This behaviour is being solved for several types (integers, etc.) in the AbstractObjectNormalizer.

My proposal is to add an option that also solves this when denormalizing nullable strings:

// Symfony\Component\Serializer\Normalizer\AbstractObjectNormalizer

    public const DENORMALIZE_EMPTY_STRINGS_AS_NULL = 'denormalize_empty_strings_as_null';

// ...

    private function validateAndDenormalize(string $currentClass, string $attribute, $data, ?string $format, array $context)
    {

// ... line 391
        if ('' === $data && 
                $type->isNullable() && 
                \in_array($type->getBuiltinType(), [Type::BUILTIN_TYPE_STRING], true) &&
                ($context[self::DENORMALIZE_EMPTY_STRINGS_AS_NULL] ?? $this->defaultContext[self::DENORMALIZE_EMPTY_STRINGS_AS_NULL] ?? false)
        ) {
                    return null;
        }
// ...

Example

Consider the following object class:

<?php

namespace App\Model;

class SomeClass
{
    /**
     * @var string
     */
    private string $foo;

    /**
     * @var string|null
     */
    private ?string $bar;

    /**
     * @return string
     */
    public function getFoo(): string
    {
        return $this->foo;
    }

    /**
     * @param string $foo
     */
    public function setFoo(string $foo): void
    {
        $this->foo = $foo;
    }

    /**
     * @return string|null
     */
    public function getBar(): ?string
    {
        return $this->bar;
    }

    /**
     * @param string|null $bar
     */
    public function setBar(?string $bar): void
    {
        $this->bar = $bar;
    }
}

And the following XML $data:

<some_class>
    <bar></bar>
    <foo></foo>
</some_class>

Or similar

<some_class>
    <bar/>
    <foo/>
</some_class>

When this data is deserialized using (current situation):

$serializer->deserialize(
    $data, 
    SomeClass::class, 
    XmlEncoder::FORMAT
);

We get a SomeClass object, with:

  • foo = "" (empty string)
  • bar = "" (empty string)

When we add the option to the context (proposed feature):

$serializer->deserialize(
    $data, 
    SomeClass::class, 
    XmlEncoder::FORMAT, 
    [
        AbstractObjectNormalizer::DENORMALIZE_EMPTY_STRINGS_AS_NULL => true
    ]
);

The result should be a SomeClass object, with:

  • foo = "" (empty string, because not nullable)
  • bar = null (because nullable)
@ro0NL
Copy link
Contributor

ro0NL commented Jun 29, 2021

As long as setBar('') is allowed at the domain level, im not sure serializer should cast (or care for that matter). In this case the empty string should be handled at the domain level IMHO ($this->prop = '' === $v ? null : $v;)

DENORMALIZE_EMPTY_STRINGS_AS_NULL => true + non-nullable property could also be expected to crash IMHO

@ogizanagi
Copy link
Member

Regarding the use-case, this feature request somehow relates to #27933 which would enable and explicit this behavior per-property (with a normalizer function converting empty strings to null).

I don't agree this is something that should happen at the domain-level. The signature of the method allowing an empty string does not mean it is expected. At best it would reject, but it does not have to adapt to the input, especially when it's a source/tool related-issue. Instead, we should tell the tool how to adapt to the expectations (either the Serializer or the PropertyAccessor component in this case).

@hafkenscheid
Copy link
Author

I like your per-property solution, and will therefore close this one.

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

No branches or pull requests

4 participants