Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

One more check #6928

Closed
wants to merge 3 commits into from
Closed

One more check #6928

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c31aa87
One more check
Will-I4M Oct 8, 2016
b12cb31
mistake on indentation
Will-I4M Oct 8, 2016
d6fefb8
simplify check
Will-I4M Oct 13, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/code/Magento/Swatches/Model/Plugin/EavAttribute.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,7 @@ protected function prepareOptionIds(array $optionsArray)
{
if (isset($optionsArray['value']) && is_array($optionsArray['value'])) {
foreach (array_keys($optionsArray['value']) as $optionId) {
if (isset($optionsArray['delete']) && $optionsArray['delete'][$optionId] == 1) {
if (isset($optionsArray['delete'][$optionId]) & $optionsArray['delete'][$optionId]) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And why not use empty? :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Contributor Author

@Will-I4M Will-I4M Oct 13, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you need I create a new simplified pull request ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry don't know how to remove the old patch, clean the history or force push.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see... I don't know a way to force push via GitHub web UI as well. Let's leave it as is then.

unset($optionsArray['value'][$optionId]);
}
}
Expand Down