Rule: Add a new rule to check constructor for OM

[jissereitsma]

Description

Rule to see if Object Manager is injected via the constructor.

Sniff checklist

• Sniff is accompanied by unit test
• Sniff is documented using the documentation template (FooSniff.md next to FooSniff.php)

[lenaorobei]

Just thought about this rule.
For MEQP we also rely on class name (variable name) as a string. This is the reason why we cannot mark such sniff with severity 10, because it's not 100% guarantee that variable $objectManager is actually an instance of ObjectManager.
PHP Codesniffer allows adding bootstrap file. In this case, there will be an opportunity to check if the specified class is instanceof ObjectManager.

[jissereitsma]

Thanks @lenaorobei for chiming in! Yeah, perhaps in my PR the warning level is much too high. We have decided earlier to discuss the warning levels of all (new and existing) rules to see whether we agree upon that. Work in progress there.

As for the technical strategy of this rule: I know about the MEQP rule, which scans for the usage of the OB. However, somebody could bypass this by using a variable $god or something. Using my approach, at least you can see the full class name, using the Reflection API to load everything. Really cool. But the downside is that it requires the loading of the autoloader, so Magento needs to be present for this to work. Otherwise, the Reflection API gives an exception and the rule doesn't continue with checking things. In short, this rule requires the presence of Magento itself.

What is missing is indeed a solid check to check up instances. Once the bootstrap file that you suggested is in place, the usage in this role of the Reflection API opens up for many cool abilities: For instance, you can scan for deprecated methods, usage of deprecated classes, quickly load other classes, etc. But we'll need a discussion first to see if this is the way to go ..

[schmengler]

What is missing is indeed a solid check to check up instances. Once the bootstrap file that you suggested is in place, the usage in this role of the Reflection API opens up for many cool abilities: For instance, you can scan for deprecated methods, usage of deprecated classes, quickly load other classes, etc. But we'll need a discussion first to see if this is the way to go ..

It's tempting, I'd also like to have these features (checking for @api is also useful for example). But IMHO we should stand by the decision we made, to not depend on a Magento installation to be present.

For things like the object manager, it's possible to resolve class names in constructor arguments (or in getInstance() classes to fully qualified class names. Again, a AST based static analysis tool (PHPStan?) would be better suited than the token based PHPCodeSniffer, but as long as we use the latter, your reflection approach seems reasonable.

I've already thought about usage of deprecated classes, and a possibility would be to hard code those into a sniff, similar to the ForbiddenFunctions sniff. There could be separate sniffs for each 2.x branch of Magento which are then included in the corresponding rulesets.

[jissereitsma]

@schmengler My problem with the earlier decision to not depend on Magento is that it does make sense from a point of view of "static code analysis". However, using Magento logic in rules (ObjectManager itself, Reflection, version detection) opens up to so many cases which aid extension quality (which is still the final goal) that I would propose the rephrase the earlier decision:

What if we say that the PHPCS rules do not require the presence of Magento. However, some rules can only easily be built when Magento is available. So perhaps, these rules should not break when Magento is not present. But when Magento is present, these rules could be "upgraded" to report much more valuable things.

Some pros & cons: It would still be possible to run PHPCS rules quickly on a code base to get a quick result (just like unit tests should be run fast). But longer, more thorough tests could be run on an instance with Magento available (just like integration tests add more value as well). This would also mean that the Travis CI that we currently use is fine for basic rules, but should be extended to run Magento instances as well (with various versions) to guarantee that rules work in various environments. This requires more work. But the analysis is something I think we would like to have somehow. The question is how to interpret this word "somehow".