Proxies and interceptors

[maderlock]

Fix #18 New rule to ensure proxies and interceptors are never explicitly requested in constructors

[paliarush]

It would be better to split this sniff into two sniffs to comply with single responsibility principle.

Or make it more generic and extensible.

[lenaorobei]

I have concerns regarding your comment:

1. This sniff covers one statement from Technical Guidelines and thats why it sounds logical to me to have this one rule.
2. Splitting into two sniffs doesn't sound good in performance prospective (make same check twice).
3. On the other hand, making it more generic sounds good to me. The name can be changed to DiscouragedDependenciesSniff (or something like this) and $warnNames property can be public and will be replaceable via ruleset.xml.

[maderlock]

3. On the other hand, making it more generic sounds good to me. The name can be changed to DiscouragedDependenciesSniff (or something like this) and $warnNames property can be public and will be replaceable via ruleset.xml.

I do like the sound of the latter - I didn't realise that this could be done. Nice :)

[paliarush]

Probably the task was not clear enough, the expectation was to prevent Interceptor classes being referenced directly. Interceptors are auto-generated classes.
It may make sense to run these tests against Magento codebase and see if Plugin reference detection also makes sense.

[maderlock]

Makes sense. I'll add interceptors first and then look at plugins as you suggest.

[paliarush]

Proxy classes are the one which have \Proxy suffix. Interceptor classes always have \Interceptor suffix. These can be accurately detected and should raise error.

Plugin classes have arbitrary names and thus are difficult to catch. These rules may have accuracy issues and can raise warnings.

[maderlock]

As this is based on names of constructor types and variables there will be a false-positive issue. In discussion with @lenaorobei it was felt that keeping these as warnings was of utility despite this.

[lenaorobei]

Even is it's bad naming it's worth pointing developers that class name with Interceptor and Proxy suffix is not a good idea.

[paliarush]

Please address suggestions above.

[maderlock]

To confirm from above:

1. Should make naming more generic and move exact types matched into ruleset.xm
2. Plugins should be removed from this sniff as this was not in the original ticket. Will look at the codeset and see if including plugins would be a useful addition.
3. Interceptors should be added instead.

[lenaorobei]

@maderlock

move exact types matched into ruleset.xm

Let them be in the sniff code. Ruleset will be an extension point (if needed in future).

@paliarush do you have additional comments?

[lenaorobei]

@maderlock thanks for your contribution!

I just had quick discussion with @paliarush and we agreed following:

1. This sniff should cover proxy and interceptor cases only. For plugins i'll create separate task.
2. Please change the severity to 10 and type to error.
3. Use more generic sniff name.
4. Please add how to fix this issue to the error message: "Proxies and interceptors MUST never be explicitly requested in constructors. Remove redundant Interceptor/Proxy suffix" (need to dynamically detect this suffix).

[maderlock]

2. Please change the severity to 10 and type to error.

I'll do as you suggest, though I'm still a little concerned about false positives given the matching of variable names.

[maderlock]

With the changes above it looks like we should no longer be detecting proxy/interceptor in variable names. The logic should now be:

• If variable type in constructor signature has a suffix of Proxy or Interceptor (or is aliased to a class matching this pattern) then this produces an error

Should we allow classes with Proxy or Interceptor in the non-suffix position, e.g. /Vendor/Module/Proxy/SomethingElse - or should this be a warning or error?

[lenaorobei]

If variable type in constructor signature has a suffix of Proxy or Interceptor (or is aliased to a class matching this pattern) then this produces an error

Yes, that's right.

Should we allow classes with Proxy or Interceptor in the non-suffix position, e.g. /Vendor/Module/Proxy/SomethingElse - or should this be a warning or error?

Non-suffix position should be fine. We dont need to trigger any warnings in this case.

To make sure everything works as expected, I suggest running this check against Magento codebase:
vendor/bin/phpcs --standard=Magento --sniffs=Magento.Classes.ConstructorProxyInterceptor /path/to/magento2/app/code. It will show possible edge cases.

[maderlock]

Sorry for the delay - I've been rather ill. I'll jump back on this when I can. Thanks for bearing with me.

[lenaorobei]

@maderlock do you need any help with this PR?

Addressed issues by rewrite

[maderlock]

Could not get properties passing from ruleset.xml, so I've had to ignore that change request. Otherwise I've made the requested changes and merged into the updated structure under Magento2 (rather than Magento).

I've tested the rule against the Magento codebase and the only errors that come out are valid ones, so I'm happy that it's working as expected (in addition to the tests passing).

[lenaorobei]

Please address my minor suggestions.

[maderlock]

Minor suggestions addressed and latest develop merged