Ability to remove key during merge #6899
Conversation
|
|
||
| namespace Zend\Stdlib; | ||
|
|
||
| class ArrayUtilsRemoveKey |
|
Agree with @Pittiplatsch here, this is exactly how a marker value should be used IMO. |
| if (array_key_exists($key, $a)) { | ||
| if (is_int($key) && !$preserveNumericKeys) { | ||
| if ($isRemove) { |
There was a problem hiding this comment.
I'd duplicate the check instead of having an assignment in here (code is very performance sensitive)
There was a problem hiding this comment.
Checking instanceof will be call always, so I don't want to duplicate it.
There was a problem hiding this comment.
Yes, I'm just suggesting a duplication here.
|
Question: how would this work if the merged configuration is cached and serialized (not uncommon with solutions like EdpSuperluminal)? |
| if (array_key_exists($key, $a)) { | ||
| if (is_int($key) && !$preserveNumericKeys) { | ||
| if ($isRemove) { |
There was a problem hiding this comment.
I think the $isRemove test should be placed as late as possible, absolutely AFTER is_int and is_array tests, as $isRemove is the most unlikely case.
There was a problem hiding this comment.
What if I want to remove key which is an int?
There was a problem hiding this comment.
You're right. Though I wonder if there is a good solution for avoiding the instanceof check for as much as possible cases...
There was a problem hiding this comment.
The instanceof check is the least impacting here
|
@weierophinney After merge array doesn't have this object. So we will serialize "standard" array |
|
@Pittiplatsch look at fixed #6903 |
|
@snapshotpl still thinking a bit about this one, but I think it's ok to go. Do you think that you can update relevant documentation bits? |
|
@Ocramius I will try. |
|
@Ocramius Where I should put docs for that? |
|
Interestingly, we don't have a chapter in the docs for the It should be somewhere near http://framework.zend.com/manual/2.3/en/modules/zend.stdlib.hydrator.html (PR should go to https://github.com/zendframework/zf2-documentation) |
|
@Ocramius |
|
@francisbesset thanks for digging it out! |
|
@snapshotpl merged, thanks!
|
…ly introduced tests
…ge-ability-to-remove-keys' into develop Close zendframework/zendframework#6899
snapshotpl
changed the title
Abilty to remove key using ArrayUtils::merge