NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[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.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

lyrixx wants to merge1 commit intosymfony:4.4fromlyrixx:validator

Closed

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

lyrixx wants to merge1 commit intosymfony:4.4fromlyrixx:validator

Conversation

Copy link

Member

lyrixx commentedDec 29, 2021

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

[Validator] Ensure a group is set on a contraint (fix BC break)

60c89d3

carsonbot added Status: Needs Review Bug Validator labels

Dec 29, 2021

carsonbot added this to the4.4 milestone

Dec 29, 2021

lyrixx requested a review fromnicolas-grekas

December 29, 2021 14:01

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

Copy link

Member

nicolas-grekas commentedDec 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)

Copy link

MemberAuthor

lyrixx commentedDec 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

Copy link

Member

nicolas-grekas commentedDec 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 in4274a1f

	if (!isset(((array)$this)['groups'])) {
	$mergedGroups = [];

	foreach ($nestedConstraintsas$constraint) {
	foreach ($constraint->groupsas$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?

Copy link

Member

nicolas-grekas commentedDec 29, 2021

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

Copy link

Member

stof commentedDec 29, 2021

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

Copy link

MemberAuthor

lyrixx commentedDec 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 mentioned this pull request

Dec 29, 2021

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

Merged

Copy link

Member

nicolas-grekas commentedDec 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

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#44847Commits-------20aacce [Validator] throw when Constraint::_construct() has not been called