One more check #6928
One more check #6928
Conversation
Without this check It's possible to have a notice undefined indice xxx -> that error is preventing me from saving an attribute in my existing config.
|
|
| if (isset($optionsArray['delete']) && $optionsArray['delete'][$optionId] == 1) { | ||
| unset($optionsArray['value'][$optionId]); | ||
| if (isset($optionsArray['delete']) && isset($optionsArray['delete'][$optionId])) { | ||
| if ($optionsArray['delete'][$optionId]== 1) { |
There was a problem hiding this comment.
A simpler condition
if (!empty($optionsArray['delete'][$optionId])) {
unset($optionsArray['value'][$optionId]);
}
will do the trick as well.
There was a problem hiding this comment.
empty() will consume more cpu cycle, and is not equivalent to isset. the purpose of this check is to avoid the notice, and it will. empty will do one more thing, it will skip the "unset" if $optionsArray['value'][$optionId] is set to '' (not possible here if it is set) .
IMHO magento 2 is slow enough to not use "simpler condition" that is actually more expensive ;)
There was a problem hiding this comment.
empty() will consume more cpu cycle
Nope, it is same to isset and always faster than isset+condition. Here we avoid three conditions with only one statement.
if $optionsArray['value'][$optionId] is set to '' (not possible here if it is set) .
Do you mean $optionsArray['delete'][$optionId] ? Please provide an exact case when behavior of new and old code differs. Surely !empty and isset are not equivalent but equally suitable for this logic.
There was a problem hiding this comment.
Please refer to the isset and empty source code (zend_isset_isempty_dim_prop_obj_handler_SPEC_UNUSED_* etc). empty is slower than isset, especially with notset arrays (~30%, and around 6% with set arrays). You use a boolean operator (!), so it's equivalent to :```
if (isset($optionsArray['delete'][$optionId]) & $optionsArray['delete'][$optionId]) {
unset($optionsArray['value'][$optionId]);
}
:).
There was a problem hiding this comment.
Exactly, and this is more effective than initial condition
if (isset($optionsArray['delete']) && isset($optionsArray['delete'][$optionId]) && $optionsArray['delete'][$optionId] == 1)
and much shorter ;)
There was a problem hiding this comment.
The PR has been updated with this optimisation.
Thanks !
| if ($optionsArray['delete'][$optionId]== 1) { | ||
| unset($optionsArray['value'][$optionId]); | ||
| } | ||
| if (isset($optionsArray['delete'][$optionId]) & $optionsArray['delete'][$optionId]) { |
There was a problem hiding this comment.
And why not use empty? :-)
There was a problem hiding this comment.
here, theoretically the binary operator is only used if the first condition has been validated. But if you insist for using "empty", with another boolean operator, it is possible, but just slower.
There was a problem hiding this comment.
I don't think such nanoseconds really matter. I usually prefer empty to avoid notices and double conditions, like with isset. For the sake of readability ;)
Whatever variant you choose, could you please squash your changes into one commit and make a force push?
There was a problem hiding this comment.
Sure !
It's not a double condition (&&) but a binary operator, like the "!" you use with empty.
Nano second, your right, but it's more than nothing, I think the code is readable :)
There was a problem hiding this comment.
Do you need I create a new simplified pull request ?
There was a problem hiding this comment.
By double condition I mean a doubled $optionsArray['delete'][$optionId] expression. So, as to me !empty variant is more readable as it is two times shorter.
No need for new PR, just force push into this branch with clean commit history.
There was a problem hiding this comment.
Sorry don't know how to remove the old patch, clean the history or force push.
There was a problem hiding this comment.
I see... I don't know a way to force push via GitHub web UI as well. Let's leave it as is then.
Without this check It's possible to have a "notice undefined indice xxx "-> thit error is preventing me from saving an attribute in my current installation.