Magento2: Implement static test to check for duplicate types in constructor signature.

[Vinai]

This issue is based on the comment from @maghamed at https://github.com/magento/magento2/issues/6114#issuecomment-290387595

To quote:

But at the same time we agreed that it's incorrect to request multiple dependencies of the same type for one class. Like here

class A 
{
    public function __construct(B $b, B $b2)
    {
        ...
    }
}

So, this new static test has to check and prevent this situation.

Preconditions

A class with a constructor duplicate object type constructor dependency

Steps to reproduce

See above.

Expected result

The static tests displays an error with a suggestion to use a Factory instead of the duplicate type dependency.

Actual result

Static test does not cover such cases

[Vinai]

Good news, the static test already (kind of) exists:
\Magento\Test\Integrity\Di\CompilerTest::testConstructorIntegrity()

The validator in question is \Magento\Framework\Code\Validator\TypeDuplication

I've tried forcing the issue by creating a class with duplicate type dependencies in a third party module, but there are two things that prevent the type duplication check from working.

The method _phpClassesDataProvider does not return all classes.
For some reason I don't understand it only returns classes that already where included, which results in only 123 core classes.

All other classes get filtered out by the class_exists($className, false) in following piece of code:

        foreach ($files as $file) {
            $file = str_replace('/', '\\', $file);
            $filePath = preg_replace($patterns, $replacements, $file);
            $className = substr($filePath, 0, -4);
            if (class_exists($className, false)) {
                $file = str_replace('\\', DIRECTORY_SEPARATOR, $file);
                $classes[$file] = $className;
            }
        }

I don't know the reason why the constructor integrity test should only be done on those 123 classes that already where included before the test is executed.

The fix is simple. by changing class_exists($className, false) to class_exists($className) all classes are validated.

The second issue is in the class \Magento\Framework\Code\Validator\TypeDuplication.
It will only validate there are no duplicate dependencies for PHP classes, not for interfaces.
The code in question is

    protected function _getObjectArguments(array $arguments)
    {
        $output = [];
        foreach ($arguments as $argument) {
            $type = $argument['type'];
            if (!$type || $type == 'array') {
                continue;
            }
            $reflection = new \ReflectionClass($type);
            if (false == $reflection->isInterface()) {
                $output[] = $argument;
            }
        }
        return $output;
    }

Again, I don't know the reasoning why this filtering takes place.

Just to try it out I modified the code so all classes and all object dependencies are validated, and only my test class caused the test to fail (as expected).

Can you provide some background information why it was decided to use class_exisis($className, false) and to filter out interface dependencies from the type duplication check?

If the reason is unknown I suggest removing those instructions so all constructors are validated, which would allow this issue to be closed.

[maghamed]

Hey @Vinai ,
I made a code review of code you mentioned and also had a conversation with authors of this code.

Looks like we could make both of changes described by you.

• change class_exists($className, false) to class_exists($className)
• eliminate this condition

            $reflection = new \ReflectionClass($type);
            if (false == $reflection->isInterface()) {
                $output[] = $argument;
            }

Short background why we got class_exists($className, false) in our codebase:
as class_exists by default makes an auto loading we didn't want this behavior for this class: /app/etc/NonComposerComponentRegistration.php (we don't have it anymore) which attempts to registers all components. However, we do not allow duplicate registration.

This change can be reverted since NonComposerComponentRegistration is no longer a class.
Related history: https://github.com/magento/magento2/commit/21a72a827ed9a82c5a5f9f837282d474cfa4e7a4

So, we can consider this as a part of legacy code we could fix.

Regarding root cause why we have a conditional logic for Interfaces which allows us to consider this as an appropriate injection:

class A 
{ 
    public function __construct(BInterface $b, BInterface $b2)
    {
        ..    
    }
}

While this one is incorrect

class A 
{ 
    public function __construct(B $b, B $b2)
    {
        ..    
    }
}

Looks like it's a bug.
And internally we agreed to fix it by elimination of this conditional logic

[maghamed]

@Vinai will you provide a PR with according changes. Looks like we resolved both of open questions

[ishakhsuvarov]

Hi @Vinai
I am closing this now due to inactivity.
Please reopen if you feel like continuing conversation on this subject.

Thank you for collaboration.

[Vinai]

Sorry for the inactivity @maghamed and @ishakhsuvarov - I missed the notification for your first reply. Too many notifications from the M2 github. Since it seems like you already have taken care of the issue I believe you did right to close it. Thanks for your work!