Require @see annotations for plugin classes and methods

[navarr]

trafficstars

Rule

• A plugin class should have a @see annotation to the class(es) that it is intercepting.
• Each plugin method should have a @see annotation to the method(s) it is intercepting

Reason

This rule would improve QOL for developers by providing a definitive link to what a plugin is intercepting. This can be especially useful for plugins that are intercepting more than one class.

Implementation

An implementation has been written by Mediotype and can be modified for this standard. Our implementation checks:

• The rule checks only a class that has a namespace like Vendor\Module\Model\Plugin\* or Vendor\Module\Plugin\*
• The rule verifies that such a class has a class-level docblock containing a @see tag
• The rule checks only methods that are public and begin with before, after, or around (case sensitive)
• The rule verifies that such methods have a method-level docblock containing a @see tag

[Vinai]

Isn't this kind of superfluous noise?

• The target class of the plugin is visible as the first argument of every plugin method (e.g. public function beforeDispatch(\Magento\Framework\App\Action\Action $subject))
• The target method is visible in every plugin method name (e.g. public function beforeDispatch => the method dispatch)

Additionally, the PHPStorm Magento Plugin adds

• an icon besides plugin classes to navigate to the di.xml line (or lines in case of multiple classes being intercepted) that declares the plugin
• an icon beside every plugin target method that allows navigating to all plugin methods for that target method.

I like to avoid all annotations or comments that don't add any new information, because

• noise to information ratio goes up (information / lines of characters)
• annotations are easy to be missed during refactorings (the annotations will often be incomplete or wrong)

[schmengler]

I agree, @Vinai . Thinking about this sentence though:

This can be especially useful for plugins that are intercepting more than one class.

In this case we should rather request the docblock to specify a union type (until we have native union types in PHP 8)

Also, I usually advice to name the plugin after the pluginized class/interface and delegate actual operations to a service with a meaningful name, especially if there are plugins for multiple methods. I don't think this should be strictly enforced by the coding standard, but it makes the described reason a non-issue.

[navarr]

@Vinai:

You may be correct. In some instances the annotation may be redundant. For plugins that are intercepting more than one class the target class cannot be declared in the signature. I think this could be resolved by updating the implementation to exclude the requirement on such lines, though there's no link that can be utilized by any IDE

The PhpStorm Magento plugin does resolve this as you've pointed out, but not all teams use PhpStorm.

@schmengler:

Also, I usually advice to name the plugin after the pluginized class/interface and delegate actual operations to a service with a meaningful name, especially if there are plugins for multiple methods. I don't think this should be strictly enforced by the coding standard, but it makes the described reason a non-issue.

This is an interesting approach. Right now I typically give the plugin itself a meaningful name and do the work in there or in another class (with the plugin prepping the parameters) depending on the size of the work that needs to be done. I've had in the past at least one scenario where I've created multiple plugins for the same class and had them separated. In your naming scenario they could not be.

[Vinai]

@navarr

For plugins that are intercepting more than one class the target class cannot be declared in the signature.

This is only true if the plugin is used to intercept the same method multiple classes that don't share a common type (e.g. an interface). I've never seen that happen. Can you give an example? I would suspect that would lead to problems anyway because even though the method name is the same, the signature very likely would be different. Using the same plugin in such a case of intercepting a method with an identical name of unrelated classes sounds like a recipe for trouble. Rather than trying to work around such issues with @see annotations, I would suggest using different plugin classes. As @schmengler and you say, business logic should not be places in plugins anyway, but in separate objects.

The final scenario you mention

I've created multiple plugins for the same class

seems unrelated to the issue at hand, i.e. the plugin subject can be part of the signatures of the plugin methods.

[Vinai]

@schmengler

I usually advice to name the plugin after the pluginized class/interface and delegate actual operations to a service with a meaningful name

Clicking on the plugin icon in PHPStorm and seeing a list of identically named plugin classes is one of my favorite pet peevs. It's really not helpul to see for example a list of ActionPlugins.

In my experience is much more helpful if the plugins are named after their purpose, e.g. TokenAuthenticationPlugin, CachingPlugin, RequestLoggingPlugin...

[schmengler]

In my experience is much more helpful if the plugins are named after their purpose, e.g. TokenAuthenticationPlugin, CachingPlugin, RequestLoggingPlugin...

I used to think the same, but I like to see from a glance, for which classes a module has plugins, and di.xml does not serve that well.

Understand your argument though. Maybe I don't use this plugin icon often enough with methods that have more than one or two plugins.

[nathanjosiah]

It's an unusual habit to have an IDE-driven design and I don't think that should be a consideration for this PR. If we could assume everyone was using PHPStorm we could switch to tabs instead of spaces and reduce our processing time and storage space by a significant amount 😉

Not considering PHPStorm or the Magento plugin for it, I do feel like this would be a helpful feature for plugins that intercept more than one class. However I also think that is a rare use case and probably a code smell by itself since that feels like a risky practice. I think if the standard "one class per plugin" paradigm is followed then the plugin is self-documenting as has been discussed.

To the point about being able to see which classes or plugins are available, this PR would not help with that.

[fascinosum]

For me, this approach is a violation of best practices - don't describe the implementation in DocBlock (method/class description). The declaration in di.xml is an implementation. If someone changes it (removes or adds plugin declaration) you will have a wrong statement in the DocBlock.

We need to remember that the plugin can be added by one module and disabled by another. If you check the di.xml declarations, you will have a clear understanding of the current state, but not with the @see annotation

[joshuaadickerson]

Don't use a plugin to wrap multiple methods. Use a single plugin with a single method for each. Then use that to call another method that contains the shared logic.

Let the code, not documentation, be the source of truth.