[2.3] [Form] Modified iterator_to_array's 2nd parameter to false in ViolationMapper #18761
Conversation
| @@ -188,7 +186,7 @@ private function matchChild(FormInterface $form, PropertyPathIteratorInterface $ | ||
| } | ||
|
|
||
| /** @var FormInterface $child */ | ||
| foreach ($children as $key => $child) { | ||
| foreach ($children as $i => $child) { |
There was a problem hiding this comment.
Is there a specific reason for this change? If not, I like the original more. $i implies that it's being used for incrementation, but you're simply referring to the key here.
There was a problem hiding this comment.
I have an intention to make the key of $children meaningless. (The name of iterator_to_array's 2nd parameter is $use_keys)
There was a problem hiding this comment.
It's used to unset something below. if you mean to make it meaningless, you should remove it altogether
There was a problem hiding this comment.
Moreover, i implies it's just counter indeed. But also implies it's an integer, IMO.
There was a problem hiding this comment.
@alekitto How do you think about this?
There was a problem hiding this comment.
I like the change: in my opinion is more clear that the variable contains an integer counter, not a meaningful key name.
Moreover it makes it consistent with the existing codebase, for example: $i is used in DI Definition::removeMethodCall method in a similar way.
There was a problem hiding this comment.
@alekitto Thank you for advicing!
|
Thank you @issei-m. |
… false in ViolationMapper (issei-m) This PR was squashed before being merged into the 2.3 branch (closes #18761). Discussion ---------- [2.3] [Form] Modified iterator_to_array's 2nd parameter to false in ViolationMapper | Q | A | ------------- | --- | Branch? | 2.3 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | n/a | License | MIT | Doc PR | n/a For #18747 Commits ------- 7101cab [2.3] [Form] Modified iterator_to_array's 2nd parameter to false in ViolationMapper
|
@issei-m Can you submit a new PR on 2.7 to update the tests? When merging, I had to revert all tests changes due to conflicts hard to resolve easily :) Thanks. |
|
nevermind, I see the other PR now :) |
For #18747