Magento 2 - good practice to use/avoid magic getters?

Question

Magic getters on Varien_Object (M1) and DataObject (M2) are common practice, but with Magento 2 it feels wrong to use it.

Good:

• easy to read/write

Bad

• It causes problems when using digits in keys (see: Magento 2: different way get field of a collection or Get Custom Product attribute using camel case)
• code analyzation tools complain about non-existing methods

Question

With Magento 2 we have two new methods:

• getDataByKey($key)
• getDataByPath($path)

Is there any good reason to still use getData($key) or any magic getters?

---

Edit:

@Vinai thanks. I did not mention the @method method, because my approach was quite different.

It only helps the IDE, but has no impact on other things.

There are several mergedf PRs on that are "micro-optimizations" like casting to (int) instead of intval() or get array size outside loops (even for small arrays).

At the other hand there are

1. magical getters, that have some "overhead" as Marius described ....

   strtolower(trim(preg_replace('/([A-Z]|[0-9]+)/', "_$1", $name), '_'));
   

2. getData($key) mehtods also have to 2-3 additional checks ...

   • if ('' === $key) {
   • if (strpos($key, '/')) {
   • if ($index !== null) {

For own code a totally agree to prefer real methods, but in same cases it's not possibly ... e.g. you've created a custom event ...

$value = $observer->getVar_1();
$value = $observer->getData('var_1');
$value = $observer->getDataByKey('var_1');

Using 3rd with /** @var some $value */ seem best to me. (?)

Answer 1

The above question is about using magic methods vs. getDataByKey or getDataByPath. I think there is a third option, too, and that is implementing real getter and setter methods.

The getData* methods all have the downside that they have to be annotated for type inference to work.
Usually that is done with a /* @var string $foo */ annotation above the getData* call.
This is a bit smelly, because the type of the data should be declared in the class that contains the data, not the class that calls getData*.
The reason for that is that if the data changes, the class is the most likely to be updated, not all getData* call sites.
That is why I think real methods increase maintainability compared to using getData* accessors.

So I think it boils down to a trade off between maintainability and quicker implementation (less code to write).

Luckily, nowadays IDEs are really good at creating the getter and setter implementations for us, so that argument doesn't really apply any more.

One more argument against magic getters and setters that is missing from the question above is that it's not possible to create plugins for them.

The only other value I think I can add to the topic is trying to collect the reasons to use or not to use @method annotations, if implementing real methods is out of the question for some reason.

Pros

• A @method annotation is a little less code to write compared to implementing a real getter and setter. This is barely true though nowadays because IDEs are good at generating accessor methods, so that's not a real benefit any more.

Cons

• It's easy for things to go wrong.
  • Annotations are comments, they become obsolete easily when code evolves but the annotations are not updated. Real methods are more robust.
  • It's possible to add multiple annotations with different type signatures without an interpreter error - static code analysis behavior is undefined, and it can lead to subtle bugs that are hard to track down.
  • If both a @method annotation and a real method with the same name exist, the annotation type signature overrides the real method during static code analysis, which is the opposite of what the PHP interpreter does. This again can easily lead to subtle bugs.

For the above reasons I personally do not use @method annotations if I can avoid them.
For code that is intended to live long I implement real getter and setter methods. The maintainability gain is worth the effort of triggering the IDE to generate them.

For more experimental code during a spike, or for a simple implementation detail of a module I use getData* methods, too, because I'm lazy.

Answer 2

The getData* methods all have the downside that they have to be annotated for type inference to work.

Usually that is done with a /*@var string $foo */ annotation above the getData* call. This is a bit smelly, because the type of the data should be declared in the class that contains the data, not the class that calls getData*.

The reason for that is that if the data changes, the class is the most likely to be updated, not all getData* call sites. That is why I think real methods increase maintainability compared to using getData* accessors.

Yes it is smelly, but can (and should?) be avoided. I think this is very common code and often suggested:

/** @var Foo $product */
$product = $model->getProduct()
if ($product->getId()) {
    $product->doSomething();
}

The problems is you just guess that the return value is of type Foo with a callable getId() methode.

For maintainability why not assume variable type and add an InvalidArgumentException?

$product = $model->getProduct()
if ($product instanceof Foo && $product->getId()) {
    $product->doSomething();
}

This also fixes static code analyzation in case that $model->getProduct() has different return types - like Foo|false. In first case it would complain about calling doSomething() on possible false.