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.

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

[RFC][Validator] Remove constraint construction from associative arrays #51393

Open
derrabus opened this issue Aug 15, 2023 · 12 comments · May be fixed by #54744
Open

[RFC][Validator] Remove constraint construction from associative arrays #51393

derrabus opened this issue Aug 15, 2023 · 12 comments · May be fixed by #54744
Labels
RFC RFC = Request For Comments (proposals about features that you want to be discussed) Validator

Comments

@derrabus
Copy link
Member

derrabus commented Aug 15, 2023 •
edited

Description

From @nicolas-grekas' comment: #51381 (comment)

For validation, I'm also wondering if the move shouldn't do one more step and clean the way we create those objects. Right now, constructors are a bit strange. Maybe relying only on named args is possible?

Currently, we have two ways of constructing our constraints: By passing all properties via named arguments or by passing one big associative array. The former is needed so we have a nice completion when using the constraint as attribute while the latter is still supported for historical reasons: Named arguments did not exist in PHP 5 and the array parameter was the ways Doctrine Annotations used to work back in the days.

new Assert\Choice(
    choices: ['fiction', 'non-fiction'],
    message: 'Choose a valid genre.',
);
new Assert\Choice([
    'choices' => ['fiction', 'non-fiction'],
    'message' => 'Choose a valid genre.',
]);

The example has been taken from https://symfony.com/doc/current/validation.html#constraint-configuration.

The code we maintain in each constraint constructor to allow both ways is pretty complex and it would make the maintenance of those classes a lot easier if we switched to named arguments completely. However, the migration path for downstream projects might be a bit bumpy.
@carsonbot carsonbot added RFC RFC = Request For Comments (proposals about features that you want to be discussed) Validator labels Aug 15, 2023
@derrabus derrabus mentioned this issue Aug 15, 2023
Closed
7 tasks
@maxbeckers
Copy link
Contributor

I like the idea to reduce the complexity in the code 👍
But of course the migration could be a big issue for some applications (and can not be done by just search & replace in the IDE). 🤔

@derrabus derrabus mentioned this issue Oct 18, 2023
Closed
@stof
Copy link
Member

stof commented Oct 18, 2023

IIRC, the Yaml and XML loaders still rely on passing all options as array in the constructor. So we would need to have an opt-in for the new behavior for each constraints.

Note that this is similar to what doctrine/annotations did in the past with their @NamedArgumentConstructor annotation allowing to opt-in an annotation into using named arguments in the constructor instead of the old way.

@norkunas
Copy link
Contributor

IIRC, the Yaml and XML loaders still rely on passing all options as array in the constructor. So we would need to have an opt-in for the new behavior for each constraints.

There is already a way #45072 :)

@stof
Copy link
Member

stof commented Oct 18, 2023

hmm, apparently, I missed the introduction of this HasNamedArguments attribute allowing to opt-in into using constructor named arguments for the Yaml and XML loaders.
So what we would need is deprecating defining constraints that don't use it and get used in YAML or XML

@derrabus
Copy link
Member Author

There's also the possibility of programmatically creating new instances of constraint classes, e.g. when declaring a form type. Migrating a large codebase that has lots of those constructor calls might be very painful, unless we have a way of automating that.

@stof
Copy link
Member

stof commented Oct 18, 2023

Migrating explicit instantiations is something that could be implemented in Rector to automate the migration (the dumb migration is to spread the array, but we can provide better output for inline arrays where the keys are known statically)

@stof
Copy link
Member

stof commented Oct 18, 2023

However, the first thing (which could even be done in 6.4) would be to ensure that all core constraints apply the attribute so that XML and Yaml loaders use the new way instead of the old one.

@derrabus
Copy link
Member Author

I have a WIP branch locally where I migrate the test suite to call constraint constructors with named arguments consitently. A very painful task. 🙉

@xabbuh xabbuh mentioned this issue Dec 21, 2023
Merged
@nicolas-grekas nicolas-grekas mentioned this issue Jan 2, 2024
Merged
@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?

@stof
Copy link
Member

stof commented Apr 19, 2024

@derrabus what is the state of your WIP branch ?

@carsonbot carsonbot removed the Stalled label Apr 19, 2024
@derrabus
Copy link
Member Author

It's WIP. With not so much P at the moment. 😓

@xabbuh xabbuh linked a pull request Apr 26, 2024 that will close this issue
Open
@xabbuh
Copy link
Member

xabbuh commented Apr 26, 2024

I gave it a try here: #54744

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
RFC RFC = Request For Comments (proposals about features that you want to be discussed) Validator
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[Validator] deprecate the use of option arrays to configure validation constraints xabbuh/symfony
6 participants
@stof @derrabus @xabbuh @norkunas @maxbeckers @carsonbot