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] fix lazy property usage. #36627

Merged
merged 1 commit into from May 2, 2020
Merged

[Validator] fix lazy property usage. #36627

merged 1 commit into from May 2, 2020

Conversation

bendavies
Copy link
Contributor

@bendavies bendavies commented Apr 29, 2020

Q A
Branch? 3.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix #36343
License MIT
Doc PR

This attempts to fix a large regression introduced in #36343, which broke recursing values returned from getter Constraints, because they are now wrapped in in a LazyProperty. The LazyProperty needs to be evaluated because some checks are done on the type of $value, i.e is_array etc... in validateGenericNode.

I'm concerned that the original PR didn't really add sufficient test coverage for the introduction of LazyProperty, and I'm not 100% sure that I've caught all the cases where the instanceof check are needed in this PR.

For the tests, I added the @dataProvider getConstraintMethods to every test that hit the problem area of code.

The only issue is that my fixed has broken the test introduced in #36343, testGroupedMethodConstraintValidateInSequence.

I think I need @HeahDude to help me work through this. Maybe there is a more simple solution, one that doesn't require doing instanceof LazyPropery checks in multiple places, because this feels very brittle.
EDIT: fixed that test.

@bendavies bendavies changed the title [Validator] fix lazy property [Validator] fix lazy property usage. Apr 29, 2020
@bendavies bendavies changed the base branch from master to 3.4 April 29, 2020 20:42
@nicolas-grekas nicolas-grekas added this to the 3.4 milestone May 1, 2020
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.

(with one minor comment)

@xabbuh
Copy link
Member

xabbuh commented May 2, 2020

Thank you @bendavies.

@xabbuh xabbuh merged commit aee10cd into symfony:3.4 May 2, 2020
This was referenced May 31, 2020
@guilliamxavier
Copy link
Contributor

FTR: Refs #36245

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