Getter methods shouldn't change values

[mzeis]

Magento 2 sometimes changes values in getter methods. This can lead to undesirable side effects.

An example from the product model:

/**
 * Get product status
 *
 * @return int
 */
public function getStatus()
{
    if ($this->_getData(self::STATUS) === null) {
        $this->setData(self::STATUS, \Magento\Catalog\Model\Product\Attribute\Source\Status::STATUS_ENABLED);
    }
    return $this->_getData(self::STATUS);
}

I'm aware this is a very general issue and has to be tackled over time during the whole code refactoring process but it is important.

[ghost]

In my opinion magento is not changing the value, it is setting the default value if there is no value.
Just like if you would have a normal class variable like:
protected $_status = \Magento\Catalog\Model\Product\Attribute\Source\Status::STATUS_ENABLED;
With the magic getter/setter magento is using I think this is a proper way to set the default value. Especially in this case where status is a mandatory field which should never be null.

[mzeis]

A default value should be set when the object is instantiated and/or the values are set, not when the value is read for the first time. The getStatus method above acts more like Schrödinger's cat and while Magento is complex it shouldn't be quantum theory complex. :-)

The behaviour was introduced in CE 1.6.1.0 and it led to a nasty bug in a third-party extension we use so I figured I might suggest this.

[alankent]

I agree with @mzeis - if it's a default, set in constructor. Sometimes getters may cache the result, and sometimes this is necessary for performance reasons. But it is risky - a source for insidious bugs. This is potentially making performance worse as every get call is doing the if statement. So in this case, no good reason for it.

That said, not sure it will be a core team priority to go on a hunt to fix them compared to other work.

[choukalos]

@mzeis - I'll write this up as a github/bug on product model ( MAGETWO-35512 ); if you/or anyone else finds more please open a separate github issue and we'll open an appropriate bug on our end to handle. We'll triage them as appropriate.

[mzeis]

Thanks! I'm happy with that.

[maksek]

@mzeis, what do you think about - call it "lazy setting"? :)

[mzeis]

Lazy setting is nice. :-) If you're into alliteration you can also call it "delayed defaulting".

[buskamuza]

I'd say that the original case and case from @Eydamos are different.
Don't think there is something bad in "lazy setting", as in @Eydamos 's example. A class has public interface, including the getter and if the getter always returns same value in the same conditions, it should be fine.
In the original case, the getter changes value that will be returned by getData() method, so the following test will fail:

$dataBefore = $obj->getData();
$obj->getStatus();
$dataAfter = $obj->getData();
$this->assertSame($dataBefore, $dataAfter);

Looks like, it's a bug in this particular implementation. If it would not impact getData(), I don't see why it can cause side effects.

Unless I'm missing something...

[choukalos]

Looks like (MAGETWO-35760) is a similar bug ( already being worked ). Linking issues.; Ignore this one.... 35760 isn't related. MAGETWO-35512 is the linked internal Jira issue for this thread.

[vpelipenko]

@mzeis, issue in getStatus() method was fixed and available from 0.74.0-beta13. Could you check it?

[mzeis]

@vpelipenko Thanks, looks good!