Skip to content

Navigation Menu

Sign in

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

[Validator] Add support for closures in When #59800

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
Merged
merged 1 commit into from
Feb 20, 2025
Merged

[Validator] Add support for closures in When #59800

merged 1 commit into from
Feb 20, 2025

Conversation

alexandre-daubois
Copy link
Member

Q A
Branch? 7.3
Bug fix? no
New feature? yes
Deprecations? no
Issues -
License MIT

Closures can be used instead of Expressions in the When constraint since PHP 8.5, like in this example:

use Symfony\Component\Validator\Constraints\NotBlank;
use Symfony\Component\Validator\Constraints\NotNull;
use Symfony\Component\Validator\Constraints\When;

class BlogPost
{
    #[When(expression: static function (BlogPost $subject) {
        return $subject->published && !$subject->draft;
    }, constraints: [
        new NotNull(),
        new NotBlank(),
    ])]
    public ?string $content;

    public bool $published;

    public bool $draft;
}

@carsonbot carsonbot added this to the 7.3 milestone Feb 18, 2025
@alexandre-daubois alexandre-daubois mentioned this pull request Feb 18, 2025
Closed
@stof
Copy link
Member

stof commented Feb 19, 2025

Wouldn't this break when caching the constraint metadata as the closure is not serializable ?

@alexandre-daubois
Copy link
Member Author

I'm not familiar with validator cache at all, where could this possibly be a problem? I couldn't break the current code when using the framework.validation.cache option

@nicolas-grekas
Copy link
Member

Looks like this option is a no-op @alexandre-daubois - it's of no use since Symfony 4 apparently.

@nicolas-grekas
Copy link
Member

That being said, it shouldn't break anything because $cache->save() will fail silently by design.
We might still want to provide a way to skip even trying to save - at least to remove some would-be-noisy logs.
@alexandre-daubois can you confirm?

@stof
Copy link
Member

stof commented Feb 19, 2025

Failing silently might still be an issue, in case the following requests consider that there is no constraints for that property (due to the failed save of the cache) instead of considering they must reload metadata without cache.

And not having a metadata cache in prod will be bad for performance.

@nicolas-grekas
Copy link
Member

And not having a metadata cache in prod will be bad for performance.

Reflection is fast nowadays, I'm not sure a cache is going to be faster than reading attributes.
A bench would be nice :)

@stof
Copy link
Member

stof commented Feb 19, 2025

@nicolas-grekas validator metadata is not only coming from attributes. We still have loaders for XML and Yaml config files (and maybe also others).

@alexandre-daubois alexandre-daubois mentioned this pull request Feb 20, 2025
Merged
nicolas-grekas added a commit that referenced this pull request Feb 20, 2025
@nicolas-grekas
feature #59814 [Framework] Deprecate the framework.validation.cache…
560a484 
… config option (alexandre-daubois)

This PR was merged into the 7.3 branch.

Discussion
----------

[Framework] Deprecate the `framework.validation.cache` config option

| Q             | A
| ------------- | ---
| Branch?       | 7.3
| Bug fix?      | no
| New feature?  | no
| Deprecations? | yes
| Issues        | #59800 (comment)
| License       | MIT

The option is no-op, I think we should deprecate it

Commits
-------

b7e4074 [Framework] Deprecate the `framework.validation.cache` config option
@alexandre-daubois
Copy link
Member Author

That being said, it shouldn't break anything because $cache->save() will fail silently by design. We might still want to provide a way to skip even trying to save - at least to remove some would-be-noisy logs.

Yes, Cache's AbstractAdapter silences any exception happening in AbstractAdapterTrait::doSave(), which happens when trying to export the constraint with the closure. There should be no logs because of this.

I'm not sure how we could skip trying to save. It seems very specific to this constraint (and Callback which should already supports closures-in-attribute I think) and it does not look like it worths it in the end?

Failing silently might still be an issue, in case the following requests consider that there is no constraints for that property (due to the failed save of the cache) instead of considering they must reload metadata without cache.

Seems that LazyLoadingMetadataFactory always tries to read metadata from the value when there's a cache miss.

@nicolas-grekas
Copy link
Member

Great, so good to merge!

chalasr
chalasr approved these changes Feb 20, 2025
View reviewed changes
Copy link
Member

@chalasr chalasr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice

@nicolas-grekas
Copy link
Member

Thank you @alexandre-daubois.

@nicolas-grekas nicolas-grekas merged commit dfd8e53 into symfony:7.3 Feb 20, 2025
9 of 11 checks passed
@nicolas-grekas nicolas-grekas mentioned this pull request Feb 20, 2025
Closed
@fabpot fabpot mentioned this pull request May 2, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@GromNaN GromNaN GromNaN approved these changes

@chalasr chalasr chalasr approved these changes

Assignees
No one assigned
Labels
Feature Status: Reviewed Validator
Projects
None yet
Milestone
7.3
Development

Successfully merging this pull request may close these issues.
6 participants
@alexandre-daubois @stof @nicolas-grekas @GromNaN @chalasr @carsonbot
Morty Proxy This is a proxified and sanitized view of the page, visit original site.