Support php7.4 nullable typed properties for JMS serializer.

[zviryatko]

In the case of php7.4 typed properties for JMS serializer there is no difference between nullable typed property and required one:

    /**
     * @Serializer\Type("integer")
     */
    private int $id;

    /**
     * @Serializer\Type("string")
     */
    private ?string $name;

Spec is generated for both properties as optional, but the $id must be required.

This PR adds support for marking fields as required automatically for php7.4 typed properties and for virtual properties based on their return type's nullability.

[zviryatko]

Oops, it seems I need to move it out to a separate controller to avoid conflicts with <7.4 tests.

[zviryatko]

It seems master branch is corrupted, so waiting for fix upstream, later I will rebase.

[shakaran]

@zviryatko could you rebase with last updates? Thanks

[zviryatko]

@shakaran finally did that 🙄

[shakaran]

@DjordyKoert could you check this for quick merge if suitable? It is already rebased

[DjordyKoert]

Sorry for the long wait! Thank you for contributing!

[zviryatko]

I've already left that job where it was needed, but the frontend dev would be happy to hear that 😅

[deluxetom]

@DjordyKoert the code for this PR is very poor, its not checking a lot of conditions:

• not checking the Property attribute if a default is set
• not checking if the defined property has a default
• no way to actually skip this auto-required feature (its better to be explicit)
• it doesn't make sense to make any non-nullable required, there's listeners, forms etc that can add defaults

[zviryatko]

Hello @deluxetom, thank you for your opinion.

not checking the Property attribute if a default is set

But is it reasonable to make property nullable if you have a default value? As I understand, you want to show in api that some property is optional for POST but how it should be described for GET? If it has a default value, then api users could rely on property that never be empty, so make it as non-nullable, but if you want to tell them that property could be null despite it has default value, then make it nullable 🤷

not checking if the defined property has a default

Not sure what the difference between the previous statement.

no way to actually skip this auto-required feature (its better to be explicit)

It shouldn't be difficult to add some if-condition to include this feature, but how it should be configured: site-wide or per model? Technically, some annotation can be added to per-model or even per-property like #[IgnoreRequired] or something, but it should be enabled by default or not. That's probably a question for the project maintainer.

it doesn't make sense to make any non-nullable required, there's listeners, forms etc that can add defaults

It really means that nothing can be not required since there are many listeners. In that case, making any generated api doc based on the model is actually a bad idea because your model doesn't describe anything.

I think that when api implementation is poor it creates so much questions and ad-hocs. But, you're welcome to add more conditions, the base part with tests already done. Or, at least you can just give more code examples with entities and desired results.

[deluxetom]

Hey @zviryatko, I'm sorry for my tone in my last comment; I didn't mean any disrespect.

My use case of this bundle creates an issue since most of the Doctrine entities are used for POST / PUT endpoints.
We use the swagger definitions to generate API clients in multiple languages, and the change to the required properties broke all of them, making almost all our properties required and breaking the forms.
The API itself didn't break, so we didn't notice this change for a few weeks.

If it has a default value, then api users could rely on property that never be empty, so make it as non-nullable, but if you want to tell them that property could be null despite it has default value, then make it nullable 🤷

The issue with this is with Doctrine, if you make a property nullable, it will make the field nullable by default, when it shouldn't. Tools like psalm-plugin-doctrine will complain :)

I'll try to add more checks, and like you said, an optional attribute to ignore this automation could also work. Since this is in the JMS describer, I didn't see a way to use a JMS attribute for this yesterday.

[zviryatko]

Ah, sorry too, just a hot morning. So, could you please give some examples of Entity definitions and desired apidoc results. Let's start from making tests and find some suitable solution to it.

[deluxetom]

Here's an example of an entity having the issue with the fields being required:

use Doctrine\ORM\Mapping as ORM;
use JMS\Serializer\Annotation as Serializer;
use OpenApi\Attributes as ApiDoc;

#[ORM\Entity]
class NotificationSettings
{
    #[ORM\Column(type: Types::DATETIME_IMMUTABLE, nullable: false, options: ['default' => 'CURRENT_TIMESTAMP'])]
    #[Serializer\Groups(['default', 'dated'])]
    #[Serializer\Accessor(getter: 'getInsertDate', setter: 'setInsertDate')]
    #[ApiDoc\Property(type: 'string')]
    public \DateTimeImmutable $insertDate;

    #[ORM\Column(type: Types::DATETIME_IMMUTABLE, nullable: false, options: ['default' => 'CURRENT_TIMESTAMP'])]
    #[Serializer\Groups(['default', 'dated'])]
    #[Serializer\Accessor(getter: 'getUpdateDate', setter: 'setUpdateDate')]
    #[ApiDoc\Property(type: 'string')]
    public \DateTimeImmutable $updateDate;

    #[ORM\Column(type: Types::BOOLEAN, options: ['default' => false])]
    #[Serializer\Groups(['default'])]
    #[ApiDoc\Property(type: 'boolean')]
    public bool $dontNotifyIfLive = false;
}

Both dates are set automatically with doctrine listeners, they're always returned but we don't need them sent to the API.
They're not nullable but they shouldn't be required.
It's almost the same with the property with a default. We don't need it sent to the API all the time, as it will use the default if it's not set.

I'm not sure how we could define the required params with the JMS attributes to be honest.
For the dates that are set automatically, maybe they should use the ReadOnlyProperty from JMS.

What do you think?

[zviryatko]

Thanks for the example.

I'm thinking that you're technically having two different models: one for POST with optional values and the second for GET with required. Those models aren't the same. But, I doubt anyone wants to define two models in such a situation, nor make property optional for both cases.

Maybe the easiest workaround would be to add some parameter to #[Property] to control is required or not, and in what cases. For example, "hasDefault=true" could provide information that the property won't be empty, but doesn't require providing the value.

The problem is that it is still two different models for POST and for GET. In swagger they must be treated as NotificationSettings1 and NotificationSettings2.

(Technically it can be achieved with virtualProperty and serializerGroup but I don't think you want to go this way)

Or it can magically detect that property is filled automatically with "default" orm/column value - which is a bad behavior, so I'd avoid such solution.

Let's ask @DjordyKoert opinion.

My final proposal is to introduce something like #[ApiDoc\Property(type: 'string', hasDefault: true)] for making OA know about value is not really required. WDYT?

[thomask]

To add to this: This change causes a (further) regression for the use_validation_groups feature. It was already broken-ish because of #2117, but this update broke it even further as it doesn't consider this setting at all and just makes anything required that does not allowNull.

On another note, maybe I'm misunderstanding, but why is the variable called $nullable and set to true here when the type or returnType !allowsNull. That seems the wrong way around.

@DjordyKoert Should this PR perhaps be reverted because of the mentioned regression? The person who made the PR seems to no longer require it, and it's breaking functionality for several people.

[DjordyKoert]

My final proposal is to introduce something like #[ApiDoc\Property(type: 'string', hasDefault: true)] for making OA know about value is not really required. WDYT?

I was already thinking about introducing something like this to simplify setting (or un-setting) required on a property #2287 (comment)

@DjordyKoert Should this PR perhaps be reverted because of the mentioned regression? The person who made the PR seems to no longer require it, and it's breaking functionality for several people.

I think the way to go is by reverting this PR

[DjordyKoert]

@zviryatko @thomask the PR has been reverted and is available in v4.29.0