Skip to content

[Validator] Ensure a group is set on a constraint (fix BC break) #44847

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
Closed
wants to merge 1 commit into from
Closed

[Validator] Ensure a group is set on a constraint (fix BC break) #44847

wants to merge 1 commit into from

Conversation

lyrixx
Copy link
Member

@lyrixx lyrixx commented Dec 29, 2021

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

@carsonbot carsonbot added this to the 4.4 milestone Dec 29, 2021
@lyrixx lyrixx changed the title [Validator] Ensure a group is set on a contraint (fix BC break) [Validator] Ensure a group is set on a constraint (fix BC break) Dec 29, 2021
@nicolas-grekas
Copy link
Member

nicolas-grekas commented Dec 29, 2021 •
edited
Loading

Are you calling the constructor? Because if not, that's the issue. The property is supposed to be lazily initialized (that's why it's unset in the constructor)

@lyrixx
Copy link
Member Author

lyrixx commented Dec 29, 2021

Are you calling the constructor?

No I don't. It used to work in previous version of symfony. And nothing force us to call the constructor (like it can be done in Command class). So to me it's a BC break and this PR solves this issue

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Dec 29, 2021 •
edited
Loading

This change is going to break Composite constraints, see

symfony/src/Symfony/Component/Validator/Constraints/Composite.php

Lines 82 to 96 in 4274a1f

if (!isset(((array) $this)['groups'])) {
$mergedGroups = [];
foreach ($nestedConstraints as $constraint) {
foreach ($constraint->groups as $group) {
$mergedGroups[$group] = true;
}
}
// prevent empty composite constraint to have empty groups
$this->groups = array_keys($mergedGroups) ?: [self::DEFAULT_GROUP];
$this->$compositeOption = $nestedConstraints;
return;
}

I understand this worked before because the property was undeclared. But to adapt to PHP 8.2, we have to declare all properties.
Using a typed property would work, but I'm not sure we can do so since we'll consider that adding the type is a BC break...

Any other idea?

@nicolas-grekas
Copy link
Member

Instead, can't we check that the property is null, meaning it need to be unset and __get again before being usable?

@stof
Copy link
Member

stof commented Dec 29, 2021

Also, I suggest trigger a deprecation saying that the parent constructor must be called.

@lyrixx
Copy link
Member Author

lyrixx commented Dec 29, 2021 •
edited
Loading

Also, I suggest trigger a deprecation saying that the parent constructor must be called.

how would you do that? with a check in all public method?

@nicolas-grekas nicolas-grekas mentioned this pull request Dec 29, 2021
Merged
@nicolas-grekas
Copy link
Member

nicolas-grekas commented Dec 30, 2021 •
edited
Loading

What about #44854 instead?
Given that this happens at compilation, I think a LogicException is fine IMHO.

ogizanagi added a commit that referenced this pull request Dec 31, 2021
@ogizanagi
bug #44854 [Validator] throw when Constraint::_construct() has not be…
d1b22ce 
…en called (nicolas-grekas)

This PR was merged into the 4.4 branch.

Discussion
----------

[Validator] throw when Constraint::_construct() has not been called

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #44846
| License       | MIT
| Doc PR        | -

Instead of #44847

Commits
-------

20aacce [Validator] throw when Constraint::_construct() has not been called
@ogizanagi
Copy link
Contributor

Superseded by #44854

@ogizanagi ogizanagi closed this Dec 31, 2021
@lyrixx lyrixx deleted the validator branch June 30, 2023 10:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas Awaiting requested review from nicolas-grekas

Assignees
No one assigned
Labels
Bug Status: Needs Review Validator
Projects
None yet
Milestone
4.4
Development

Successfully merging this pull request may close these issues.
5 participants
@lyrixx @nicolas-grekas @stof @ogizanagi @carsonbot