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

[Validator] Accept Stringable in ExecutionContext::build/addViolation() #54487

Merged
merged 1 commit into from
Apr 5, 2024

Conversation

alexandre-daubois
Copy link
Contributor

@alexandre-daubois alexandre-daubois commented Apr 4, 2024

Q A
Branch? 6.4
Bug fix? yes
New feature? no
Deprecations? no
Issues Fix symfony/symfony-docs#19704
License MIT

ConstraintViolationBuilder accepts any stringable. We mentioned this in the doc but it doesn't work as explained in the issue. I think buildViolation() could accept stringable, also given that ExecutionContextInterface defines the type as string|Stringable in the PHPDoc.

Same for addViolation().

@carsonbot carsonbot added this to the 6.4 milestone Apr 4, 2024
@alexandre-daubois alexandre-daubois changed the title [Validator] Accept Stringable in `ExecutionContext::build/addViolat… [Validator] Accept Stringable in ExecutionContext::build/addViolation() Apr 4, 2024
@nicolas-grekas
Copy link
Member

Just to be sure: this matters only when strict types are enabled, right? Looking at the @param in the interface, I'd say we might have forgotten to change the type there in 7.0. Let's do it on 7.0 as a bugfix?

@alexandre-daubois
Copy link
Contributor Author

Indeed! It works as-is when strict types are disabled. I guess we have to wait for the next major to update the interface, even if it's a bug?

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Apr 4, 2024

Ah no sorry, the change was fine for 6.4.
For 7.0, that'd be on the interface.
We might want to check other @param in case we forgot to update a few others. Not sure how.

@alexandre-daubois alexandre-daubois changed the base branch from 7.0 to 6.4 April 4, 2024 11:49
@alexandre-daubois
Copy link
Contributor Author

Got back to 6.4. 👍

I think this is something that may be doable with some custom PHPStan rule? I didn't play with this yet, that's an interesting case. I'll try something when I have some time.

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.

Note that strict types are just a bad idea :)

@fabpot
Copy link
Member

fabpot commented Apr 5, 2024

Thank you @alexandre-daubois.

@fabpot fabpot merged commit 9c4adba into symfony:6.4 Apr 5, 2024
16 of 18 checks passed
This was referenced Apr 29, 2024
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.

None yet

5 participants