Fix merge direction #1571
Fix merge direction #1571
Conversation
| @@ -91,9 +91,9 @@ public function fixFieldDescription(FieldDescriptionInterface $fieldDescription) | |||
| ModelFilter::class === $fieldDescription->getType() && null === $fieldDescription->getOption('field_type') | |||
| || EntityType::class === $fieldDescription->getOption('field_type') | |||
| ) { | |||
| $fieldDescription->mergeOption('field_options', [ | |||
| $fieldDescription->setOption('field_options', array_merge([ | |||
There was a problem hiding this comment.
If these offsets are strings, we could use + instead of array_merge().
There was a problem hiding this comment.
Do you prefer $a + $b over array_merge($b, $a) or do we merge like this ?
There was a problem hiding this comment.
IMO, avoiding a function call is better for performance reasons.
There was a problem hiding this comment.
I think array_merge is clearer and more descriptive.
Performance wise I've just found this: https://stackoverflow.com/questions/57192782/array-merge-vs-direct-array-injection-performance/57193666 which indicates that array_merge is faster.
Edit: Sorry, I've skipped a few lines. This doesn't compare with +.
There was a problem hiding this comment.
It's just a suggestion, not a blocker for the fix.
We could make performance improvements in a new PR.
For the records: https://gist.github.com/Ocramius/8399625.
There was a problem hiding this comment.
@phansys not sure it's still true, the comparison was done with PHP 5.4
|
Thanks |
Subject
This PR fixes an issue with non mapped filter made with
CallbackFilter.I am targeting this branch, because this bug was introduced and isn't present in any other branch.
Closes #1568
Changelog