Variable as a method parameter might be overridden by the loop

[lfluvisotto]

Description

Variable as a method parameter might be overridden by the loop.
Rename the variable in the loop let the code more readable.

[magento-engcom-team]

Hi @lfluvisotto. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

• @magento-engcom-team give me test instance - deploy test instance based on Pull Request changes
• @magento-engcom-team give me new test instance - deploy NEW test instance based on Pull Request changes
• @magento-engcom-team give me {$VERSION} instance - deploy Vanilla Magento instance for Issue or Pull Request

For more details, please, review the Magento Contributor Assistant documentation

[orlangur]

This makes code LESS readable. Never like such underscore-prefixed variables in phtml templates, it goes against the spirit of PSR-2.

public function updateQtyOption($options, \Magento\Framework\DataObject $option, $value, $product)
    {
        $optionProduct = $option->getProduct($product);
        $optionUpdateFlag = $option->getHasQtyOptionUpdate();
        $optionCollection = $this->getOptionsCollection($product);
        $selections = $this->getSelectionsCollection($optionCollection->getAllIds(), $product);
        foreach ($selections as $selection) {
            if ($selection->getProductId() == $optionProduct->getId()) {
                foreach ($options as &$_option) {

There is no risk in overriding $option here as it is not passed by reference into function. Didn't check other occurrences, please provide all of them containing a real problem.

[lfluvisotto]

@orlangur less readable? Sorry, I don't think so.

I understand that each developer has its way of developing, but I think this question has to be in the consensus of other developers who shares the same opinion.

It is a improvement to the code, there is no problem.

I think Magento also cares about improving the current code.

About https://www.php-fig.org/psr/psr-2/ in loop foreach is about the placement of
parentheses, spaces, and braces, but not about the variable names.

I underscored the variable name in the loop which means that variable scope it is just for loop (local).

[lfluvisotto]

Also @sidolov what is your opinion? what do you think? it's relevant or not?

[lfluvisotto]

Also @okorshenko what is your opinion? what do you think? it's relevant or not?

[orlangur]

@lfluvisotto please answer on "Didn't check other occurrences, please provide all of them containing a real problem.".

Underscores were discouraged in PSR-2 to indicate visibility in multiple contexts. As I said, "goes against the spirit of PSR-2", of course there is no specific rules for variable naming.

[lfluvisotto]

@orlangur

Do you agree if I rename those variables to other names without underscore?

[orlangur]

@lfluvisotto,

please provide all of them containing a real problem

Only in places where there is a real problem - like passing method argument by reference. There is no need to change 21 files if only one is really affected. In such case tool you used just gave a false-positive result.

[lfluvisotto]

I will work on the changes tonight.

[lfluvisotto]

Got a little free time.

@orlangur "Only in places where there is a real problem - like passing method argument by reference"

Ok, done.

[orlangur]

$option->setValue(intval($option->getValue()));

Oh, that was weird.

Could you tell me from logic, optionSelection is not really clear to me, is it selectedOption or selectionOption (i.e. "option of selection")? Please rename variable accordingly.

[lfluvisotto]

@orlangur I've renamed as $quoteItemOption, I think is clear now.

The call comes from app/code/Magento/Quote/Model/Quote/Item.php:662

$this->getOptions() returns \Magento\Quote\Model\Quote\Item\Option[]

[orlangur]

@lfluvisotto really nice! 👍

First time I met in Magento when static analysis caught a real bug :)

[lfluvisotto]

https://secure.phabricator.com/book/phabflavor/article/php_pitfalls/

Following the tip below:

The easiest way to avoid this is to avoid using foreach-by-reference. If you do use it, unset the reference after the loop:

foreach ($array as &$value) {
// ...
}
unset($value);

[orlangur]

Why do we need foreach by reference here at all? It is an object, please just remove ampersand and unset.

[orlangur]

@lfluvisotto please squash it and then I'll give an approval in no time.

[lfluvisotto]

@orlangur everything just in one commit is that you need?

[orlangur]

@lfluvisotto exactly! 👍

[magento-engcom-team]

Hi @orlangur, thank you for the review.
ENGCOM-2176 has been created to process this Pull Request

[magento-engcom-team]

Hi @lfluvisotto. Thank you for your contribution.
We will aim to release these changes as part of 2.2.6.
Please check the release notes for final confirmation.

Please, consider to port this solution to 2.3 release line.
You may use Porting tool to port commits automatically.