Rule: No Object Manager in classes, except for factories, proxies, builders

[jissereitsma]

The Object Manager should not be injected directly through constructor DI into a class, except for when that classes appears to be a factory, proxy, builder or similar pattern.

[renttek]

What about the use of ObejectManager::getInstance and the new keyword ?

[jissereitsma]

The call ObjectManager::getInstance gives you an instance of the Object Manager, which is already mentioned by the rule topic. So, yes. As for the new keyword, this is different discussion and would need a new issue instead.

[ihor-sviziev]

Why for factories, proxies, builders? You mean just inside those classes object manager can be used? Or for creation factory we can use object manager? If second option - I'm totally disagree

[jissereitsma]

Yes, I meant the first. Whenever, you create a factory, proxy or builder class, there is an injection of the object manager in its constructor. For reference, see the source code of Magento its core factories and proxies (generated or not). The same applies to builders. So the design pattern more or less makes it legal to inject the object manager. For other classes, there is no such pattern.

I'm not sure what you mean with creation factory. Do you have a code sample for this?

[schmengler]

I guess, "for creation factory" was meant as "to create a factory".

@ihor-sviziev this was not the intention. Just that if you create a custom factory, proxy or builder, you are allowed to use the object manager in there.

Technically the rule is: never use "ObjectManager" except in classes with one of these suffixes:

• Factory
• Proxy
• Builder

(did I miss any?)

[larsroettig]

@schmengler i will work on this

[fooman]

I guess ObjectManager usage should also be allowed in tests.

[fooman]

But maybe it's a broader discussion if the ruleset should even run over tests at all (maybe it just falls back to PSR-2 for example).

[larsroettig]

@fooman i will check the include ObjectManager must be allowed for unit tests

[navarr]

Magento themselves care about coding standards in tests - at least for PHPCS/PHPMD - so this probably should too

[lenaorobei]

Maybe it'll be helpful https://github.com/magento/marketplace-eqp/blob/master/MEQP2/Sniffs/Classes/ObjectManagerSniff.php

There are no exceptions in this sniff, but they can be easily added.

[jissereitsma]

@lenaorobei The EQP2 rule mentioned could be included as well, but simply checks whether a variable with the name objectmanager (or something) is used in the core.

To all, I have now created a PR for a rule that checks the constructor instead: #48

[jissereitsma]

I have added a label for debate, because I have added some additional utilities (Utils/) in this PR that use the Reflection API to allow for the class constructor to be inspected in a more flexible way. I'm not sure if that's the path to go for.

[schmengler]

As discussed in the hangout, please go forward with this approach.