[Config] Enable cannotBeEmpty along with requiresAtLeastOneElement #20361
Conversation
| @@ -117,9 +117,13 @@ public function addDefaultChildrenIfNoneSet($children = null) | |||
| * This method is applicable to prototype nodes only. | |||
| * | |||
| * @return ArrayNodeDefinition | |||
| * | |||
| * @deprecated Deprecated since version 3.2, to be removed in 4.0. Use cannotBeEmpty() instead. | |||
|
Otherwise what about disallowing |
| @@ -372,7 +376,7 @@ protected function createNode() | |||
| $node->setKeyAttribute($this->key, $this->removeKeyItem); | |||
| } | |||
|
|
|||
| if (true === $this->atLeastOne) { | |||
| if (true === $this->atLeastOne || false === $this->allowEmptyValue) { | |||
There was a problem hiding this comment.
or given this bug, shouldn't we not fix it (to prevent any bc break), and deprecate cannotBeEmpty instead?
There was a problem hiding this comment.
Fine either way... the numeric node does that actually.
Personally i'd favor Array::cannotBeEmpty over requiresAtLeastOneElement, but maybe that's just me :)
Maybe provide requiresAtLeastOneElement as an alias to cannotBeEmpty?
There was a problem hiding this comment.
I'd favor simplicity her, taking history into account, thus stick to requiresAtLeastOneElement and make the PR as small as possible. It's not like requiresAtLeastOneElement is a bad name.
There was a problem hiding this comment.
Agree. Deprecating cannotBeEmpty means it should throw in 4.0 right?
There was a problem hiding this comment.
or just be removed if we can
There was a problem hiding this comment.
Updated.
Removed the deprecation (worth it?) so both requiresAtLeastOneElement and cannotBeEmpty act the same. We cannot really deprecate/remove cannotBeEmpty as it comes from the base class (not sure this should be reversed in any way from a sub class).
So either deprecate requiresAtLeastOneElement in favor of cannotBeEmpty or support both like i did.
Or indeed throw from ArrayNode::cannotBeEmpty.. but im not sure it's worth the hassle.
| @@ -117,9 +117,13 @@ public function addDefaultChildrenIfNoneSet($children = null) | |||
| * This method is applicable to prototype nodes only. | |||
| * | |||
| * @return ArrayNodeDefinition | |||
| * | |||
| * @deprecated Deprecated since version 3.3, to be removed in 4.0. Use cannotBeEmpty() instead. | |||
|
Status: needs work |
|
Tests added.
Agree. Not sure we should do anything on lower branches given "ignored method call" vs. "crashing method call". Right now both Status: needs review |
|
@fabpot in case you missed it (failures unrelated). Let me know if i should add a changelog or so. |
There was a problem hiding this comment.
LGTM
@ro0NL can you please rebase to trigger tests again so that we can see them green?
|
All good. Failure unrelated. |
|
ping @ogizanagi @fabpot |
|
Thank you @ro0NL. |
…stOneElement (ro0NL) This PR was squashed before being merged into the 3.4 branch (closes #20361). Discussion ---------- [Config] Enable cannotBeEmpty along with requiresAtLeastOneElement | Q | A | | --- | --- | | Branch? | "master" | | Bug fix? | no | | New feature? | yes | | BC breaks? | no | | Deprecations? | no | | Tests pass? | yes | Fixed tickets | #20356 | | License | MIT | | Doc PR | reference to the documentation PR, if any | As @vudaltsov mentioned, we ignore any calls to `ArrayNodeDefinition::cannotBeEmpty`, which can lead to unexpected behavior. Imo. all subclasses should follow the base API. Commits ------- d40e7e4 [Config] Enable cannotBeEmpty along with requiresAtLeastOneElement
As @vudaltsov mentioned, we ignore any calls to
ArrayNodeDefinition::cannotBeEmpty, which can lead to unexpected behavior. Imo. all subclasses should follow the base API.