[Validator] return empty metadata collection if none do exist #11614
Conversation
xabbuh
commented
Aug 8, 2014
| Q | A |
|---|---|
| Bug fix? | yes |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | #11604 |
| License | MIT |
| Doc PR |
|
The |
|
The same check is needed in |
|
@stof Thanks, you're right. Updated it. |
The PropertyMetadataContainerInterface defines that the method getPropertyMetadata() has to return an empty collection if no metadata have been configured for the given property. Though, its implementation in the ClassMetadata class didn't check for existence of such metadata. This behavior led to unexpected PHP notices when validating a property or a property value of a property without any configured constraints (only affects the new 2.5 API). Additionally, the getMemberMetadatas() didn't check for existing array keys as well which has also been fixed.
|
👍 ping @symfony/deciders |
| @@ -371,6 +371,10 @@ public function hasMemberMetadatas($property) | ||
| */ | ||
| public function getMemberMetadatas($property) |
There was a problem hiding this comment.
Why do we need this method when it has the same implementation as getPropertyMetadata?
There was a problem hiding this comment.
getPropertyMetadata() was introduced in efe42cb. I guess getMemberMetadatas() was kept for BC.
There was a problem hiding this comment.
shouldn't it be @deprecated then?!
There was a problem hiding this comment.
it is deprecated at the interface level. This class implements both the new interface and the deprecated one
There was a problem hiding this comment.
indeed, looks like a left-over of an even older implementation
There was a problem hiding this comment.
I agree that it should be deprecated. Though not in this pull request I guess. Should I add it in #11615 or open another PR for it?
There was a problem hiding this comment.
we should only deprecate things in master, not in 2.3
…exist (xabbuh) This PR was merged into the 2.3 branch. Discussion ---------- [Validator] return empty metadata collection if none do exist | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | [The reference to the documentation PR if any] Backport of #11614 for Symfony 2.3 and 2.4. Commits ------- f5bc18d return empty metadata collection if none do exist
|
Merged into 2.3. |
|
@webmozart this PR was adding an extra test. Is it worth adding it ? |
|
@webmozart In addition to #11615, this also contains test for the |
|
@xabbuh Indeed. Would you mind opening a new PR with these tests? |
|
@xabbuh the question is whether this test is worth it now that we have the test for the ClassMetadata itself |
|
@stof I think it is. |
|
@stof The question is if we want to keep it to avoid regressions. There may be other reasons in the future for the |
|
@webmozart see #11707 |
…straint is defined (xabbuh) This PR was merged into the 2.5 branch. Discussion ---------- [Validator] Test that validateProperty() works if no constraint is defined | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes (2.3 has to be merged into 2.5 first) | Fixed tickets | #11604, #11614 | License | MIT | Doc PR | Adds a test case for #11604 to avoid regressions. The actual issue has been fixed in Symfony 2.3 with the merge of #11615. Commits ------- a47a884 add test for #11604