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

Add is_valid function to Expression constraint #50438

Merged

Conversation

DEVizzent
Copy link

Q A
Branch? 6.4 for features
Bug fix? no
New feature? yes
Deprecations? no
Tickets Fix #11940
License MIT
Doc PR TODO

Add is_valid function to the Expression constraint, api the same as ValidatorInterface::validate

@carsonbot
Copy link

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has a contribution guide which I suggest you to read.

In short:

  • Always add tests
  • Keep backward compatibility (see https://symfony.com/bc).
  • Bug fixes must be submitted against the lowest maintained branch where they apply (see https://symfony.com/releases)
  • Features and deprecations must be submitted against the 6.4 branch.

Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change.

When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

@carsonbot
Copy link

Hey!

Thanks for your PR. You are targeting branch "6.4" but it seems your PR description refers to branch "6.4 for features".
Could you update the PR description or change target branch? This helps core maintainers a lot.

Cheers!

Carsonbot

@DEVizzent DEVizzent force-pushed the feature/validator-expression-is_valid branch from be02096 to c20d5cb Compare May 25, 2023 22:10
@xabbuh
Copy link
Member

xabbuh commented May 26, 2023

Is this PR meant to replace #47153?

@DEVizzent
Copy link
Author

Is this PR meant to replace #47153?

I'm trying to do it. I saw that ones was stuck for long time. I rebased, but I miss a }, I will work on it this weekend.

@DEVizzent DEVizzent force-pushed the feature/validator-expression-is_valid branch from c20d5cb to c731b88 Compare May 27, 2023 06:13
@DEVizzent
Copy link
Author

Hi @xabbuh ,

Sorry but I don't find the error in the continuous-integration/appveyor/pr. Can you bring me any advice to identify why it's failling?

Thank you so much 😃 When I fix it I think we can replace #47153 for this one.

@xabbuh
Copy link
Member

xabbuh commented Jun 1, 2023

@DEVizzent I don't think the failing job is related to your changes.

@DEVizzent DEVizzent force-pushed the feature/validator-expression-is_valid branch from 1338669 to 5084bd2 Compare June 4, 2023 06:20
@DEVizzent
Copy link
Author

@DEVizzent I don't think the failing job is related to your changes.

This time most of jobs have failed, but I only change a CHANGE.log in a commit...
If I did something wrong, let me know and I will try fix it. ;)

@DEVizzent DEVizzent force-pushed the feature/validator-expression-is_valid branch from 5084bd2 to f8b8a1c Compare July 20, 2023 20:19
@DEVizzent
Copy link
Author

Hi @xabbuh
I have updated the branch with 6.4, It's failing the same that in the 6.4 pipeline (https://github.com/symfony/symfony/actions/runs/5613675664). Can I do anything more to get the approval and merge it?

@nicolas-grekas nicolas-grekas changed the title Feature/validator expression is valid [Validator] Add is_valid function to Expression constraint Sep 13, 2023
@nicolas-grekas nicolas-grekas force-pushed the feature/validator-expression-is_valid branch from f8b8a1c to 3d1a798 Compare September 13, 2023 09:04
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

I rebased the PR and simplified the implementation a bit.

@carsonbot carsonbot changed the title [Validator] Add is_valid function to Expression constraint Add is_valid function to Expression constraint Sep 13, 2023
@nicolas-grekas
Copy link
Member

Thank you @DEVizzent.

@nicolas-grekas nicolas-grekas merged commit 30a35e4 into symfony:6.4 Sep 13, 2023
6 of 9 checks passed
@nicolas-grekas
Copy link
Member

And thank you @verdet23 !

@bendavies
Copy link
Contributor

bendavies commented Sep 26, 2023

We tried our 6.3 app on 6.4.x-dev

With an empty cache in a symony app, when is_valid is added, this causes

"Registering functions after calling evaluate(), compile() or parse() is not supported." at ExpressionLanguage.php line 122 {"exception":"[object] (LogicException(code: 0): Registering functions after calling evaluate(), compile() or parse() is not supported. at /vendor/symfony/expression-language/ExpressionLanguage.php:122)

because you are cloning and then adding a function an already cached/parsed ExpressionLangauge object.

@bendavies
Copy link
Contributor

bendavies commented Sep 26, 2023

@nicolas-grekas possibly it the clone was your change? i'm finding it hard to tell from the force pushes/rebases.

@nicolas-grekas
Copy link
Member

@nicolas-grekas possibly it the clone was your change?

Correct, I missed this. Can you submit a fix?

@bendavies
Copy link
Contributor

bendavies commented Sep 26, 2023

I can but I do not understand the desired functionality.
Why do we need a new ExpressionLanguage?
Why are you cloning?
What do you see the fix being?

@nicolas-grekas
Copy link
Member

The idea is to not mutate the injected service.
The solution might be to pass a pre-configured service I suppose.

@bendavies
Copy link
Contributor

bendavies commented Sep 26, 2023

are you suggesting a new service (same as this one) which ExpressionValidator uses instead?

thanks

nicolas-grekas added a commit that referenced this pull request Sep 27, 2023
…]` (nicolas-grekas)

This PR was merged into the 6.4 branch.

Discussion
----------

[Validator] Fix registering "is_valid()" for `#[Expression]`

| Q             | A
| ------------- | ---
| Branch?       | 6.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | #50438
| License       | MIT
| Doc PR        | -

`@bendavies` can you please confirm this fixes the issue for you?

Commits
-------

b24d9a0 [Validator] Fix registering "is_valid()" for `#[Expression]`
This was referenced Oct 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Validator] Validate from within Expression
6 participants