Interface constructor if present will break Magento compilation

[redboxmarcins]

Preconditions

1. Install latest Magento 2.x (tested on 2.0.x and 2.1.x) - PHP 5.6 or PHP 7.0
2. Any custom module with interface defined

Steps to reproduce

1. Create module and define Interface with __construct method
2. Run php bin/magento setup:di:compile

Expected result

1. Compilation should complete 100% as PHP allows interface to have constructor - I'm not saying it should have, but it can as language allows it - so compiler should honor that

Actual result

1. Error: PHP Notice: Trying to get property of non-object in /var/www/html/lib/internal/Magento/Framework/Code/Reader/ArgumentsReader.php on line 30

Problem is here

magento2/lib/internal/Magento/Framework/Code/Reader/ArgumentsReader.php

Line 28 in b1e56ed

if (!$class->getFileName() || false == $class->hasMethod(

If class hasn't got '__construct' method it will be skipped, but if method exist

$class->getConstructor() will return NULL
and
$class->getConstructor()->class obviously will generate fatal error

Simplest fix

if (!$class->getFileName() || false == $class->hasMethod(
            '__construct'
        ) || !$inherited && null !== $class->getConstructor() && $class->getConstructor()->class != $class->getName()
        ) {
            return $output;
        }

[maghamed]

@redboxmarcins make sense,
moreover looks like we can't instantiate Objects without constructor not firing E_NOTICE
class SomeClass {

function funcA($arg1, $arg2) {

}

}

$refl = new ReflectionClass('SomeClass');

var_dump($refl->isInstantiable()); // bool(true)
var_dump($refl->getConstructor()); // NULL

[orlangur]

Strongly disagree.

Please take a look on some http://stackoverflow.com/a/689488 and http://stackoverflow.com/a/41685659.

Compilation should complete 100% as PHP allows interface to have constructor - I'm not saying it should have, but it can as language allows it - so compiler should honor that

First of all, Magento in general and its compiler in particular should honor and encourage best practices. So, when PHP design flaws we should mitigate problems instead of providing support. It is a good thing to disallow shooting yourself in the foot whenever it is possible.

Obviously, compiler must not display nonsense like PHP Notice: Trying to get property of non-object but some LogicException throwing would be much better than supporting edge cases caused by poor PHP design.

[maghamed]

@orlangur
one more time, it make sense for us because for now instantiation of class without constructor will fire E_NOTICE.
Also Framework components of Magento should follow Defensive programming principles as it could/gonna be used for applications besides Magento.

[orlangur]

@maghamed, I have nothing against defensive programming,

for now instantiation of class without constructor will fire E_NOTICE.

Hope you are not going to add check for

var_dump($refl->isInstantiable()); // bool(true)
var_dump($refl->getConstructor()); // NULL

? Notice in this case looks perfectly valid to me as it allows to fail fast.

Anyway, this issue was already fixed and delivered to develop in #8232 the way I like it (NO interface with constructor allowed).

[maghamed]

@orlangur one more time,

current implementation with the fix you mentioned would not allow you to instantiate such class without E_NOTICE:

class SomeClass {
function funcA($arg1, $arg2) {
}
}

Proposed fix -
null !== $class->getConstructor()
make a lot of sense for us.

In any case contract of getConstructor() could return NULL, so next chain access
$class->getConstructor()->class
is not allowed without making sure that null !== $class->getConstructor()

That's one of the Defensive Programming principle.

[orlangur]

In any case contract of getConstructor() could return NULL, so next chain access
$class->getConstructor()->class

Okok, if this code is still reachable for $class->getConstructor() === null additional check won't hurt.

I was saying that I'm against such check in a place where we actually instantiate such object, like https://github.com/magento/magento2/blob/develop/lib/internal/Magento/Framework/ObjectManager/Factory/AbstractFactory.php#L91

[vrann]

@maghamed is this PR related? #8232

[orlangur]

@vrann, yeah,

Anyway, this issue was already fixed and delivered to develop in #8232 the way I like it (NO interface with constructor allowed).

If I understood @maghamed correctly, he wants to add one more check for better defense in scope of this issue.