Skip to content

[Validator] Added StrictTypes as class constraint with not nullable typed properties #36494

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

[Validator] Added StrictTypes as class constraint with not nullable typed properties #36494

wants to merge 2 commits into from

Conversation

HeahDude
Copy link
Contributor

Q A
Branch? master
Bug fix? no
New feature? yes
Deprecations? no
Tickets ~
License MIT
Doc PR TODO

After I commented #36352 (comment), I started to work on a proper constraint instead. Following #35532, #36492 and #36352, this PR enables the following short example:

  • Before: 
    /**
 * @Assert\Cascade
 */
class User
{
    /**
     * @Assert\NotNull
     * @Assert\Regex("some pattern")
     */
    public string $username;

    /**
     * @Assert\NotNull
     * @AssertEmail
     */
    public string $email;

    /**
     * @AssertEmail
     */
    public ?string $secondaryEmail;

    /**
     * @Assert\NotNull
     */
    public Address $address;

    public ?Address $secondaryAddress;
}
  • After: 
    /**
 * @Assert\Cascade
 * @Assert\StrictTypes
 */
class User
{
    /**
     * @Assert\Regex("some pattern")
     */
    public string $username;

    /**
     * @AssertEmail
     */
    public string $email;

    /**
     * @AssertEmail
     */
    public ?string $secondaryEmail;

    public Address $address;

    public ?Address $secondaryAddress;
}

Until #36352 is merged or close, only the second commit should be reviewed here.

Side note: unlike AutoMapping, this feature require PHP 7.4, but do not rely on complexity or other components to achieve a simple non nullable check.
This remains explicit and works for simple VO which do not require PHP annotations anymore, nor ORM mapping.

@HeahDude HeahDude force-pushed the validator/strict_types branch from 333ca17 to 9cf7cbd Compare April 19, 2020 14:31
@nicolas-grekas nicolas-grekas added this to the next milestone Apr 19, 2020
@ro0NL
Copy link
Contributor

ro0NL commented Apr 20, 2020

how can public string $username; ever be null in the first place?

@HeahDude
Copy link
Contributor Author

@ro0NL when hydrating objects from external source/input, e.g a submitted form or a deserialized payload to be validated.

@ro0NL
Copy link
Contributor

ro0NL commented Apr 20, 2020

AFAIK you'll hit Uncaught TypeError: Typed property Class::$prop must be string, null used in general. The only reason this works is due PropertyMetadata::getPropertyValue i figured.

Perhaps the ship has sailed, and this complements our PHP74 support in general. But IMHO one should validate the raw payload, before hydrating a non-nullable property.

@HeahDude
Copy link
Contributor Author

Not in the context of the form given #36492.
But I agree for validating the raw payload as a preference, however this is still a shortcut for not null properties that could be read as null for partially hydrated objects or when relying on https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Validator/Validator/ValidatorInterface.php#L55 to validate the raw payload.

This allows to decouple model from hydrators, and explicitly declare not nullable props with the type instead of a not null constraint.
The naming is bit confusing though. What about @Assert\NotNullableTypes?

@HeahDude
Copy link
Contributor Author

Or @Assert\Initialized?

@ro0NL
Copy link
Contributor

ro0NL commented Apr 20, 2020

does that mean Assert\Initialized still translates to NotNull constraints, based on the fact we also get null from properties in uninitialized state? Doesnt that trigger side effects if the value is truly null?

In general i like Assert\Initialized as a class constraint, to ensure all properties are initialized.

However, IMHO it's developer concern. De facto, objects shouldnt be in uninitialized state after construct.

@fabpot
Copy link
Member

fabpot commented Aug 18, 2020

I'm not sure I like this StrictTypes constraint. What do you think @xabbuh?

@fabpot
Copy link
Member

fabpot commented Sep 7, 2020

Closing for now as the PR is stale. Feel free to reopen when you have time. Thank you.

@fabpot fabpot closed this Sep 7, 2020
@nicolas-grekas nicolas-grekas modified the milestones: next, 5.2 Oct 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Feature Status: Needs Review Validator
Projects
None yet
Milestone
5.2
Development

Successfully merging this pull request may close these issues.
5 participants
@HeahDude @ro0NL @fabpot @nicolas-grekas @carsonbot