nystudio107 / generator-craftplugin

generator-craftplugin is a Yeoman generator for Craft CMS plugins
MIT License
73 stars 30 forks source link

Unnecessary static property: instance/plugin #85

Closed timkelty closed 5 years ago

timkelty commented 5 years ago

Both Plugin::$plugin and Module::$instance get set to $this in the init methods.

Yii already provides a Module::getInstance(). Seems like unnecessary overhead to include this prop.

putyourlightson commented 5 years ago

This is a convention that many plugin developers, myself included, have adopted. It is purely for readability purposes (and brevity) and follows the Craft::$app convention. Compare how you call the Blitz plugin versus the Commerce plugin, for example:


use putyourlightson\blitz\Blitz;
use craft\commerce\Plugin;
...

// Call the Blitz plugin
Blitz::$plugin

// Call the Commerce plugin
Plugin::getInstance()
timkelty commented 5 years ago

@putyourlightson Gotcha, makes sense.

I guess personally I'd lobby for simplicity and keeping "preferences" like this out of the generator.

I don't really see a win between Blitz::$plugin and Blitz::getInstance(), but that's just me :)

putyourlightson commented 5 years ago

I guess the argument could be made that calling the getInstance() method several times is less performant than calling it once, when $plugin is first defined. But it is an insignificant optimisation, so really just a matter of preference.

lindseydiloreto commented 5 years ago

@timkelty I've waffled on this myself. I think we can all agree that Blitz:: is better than Plugin::, but then (as Ben said) it's just personal preference as to whether you use ::$plugin or ::getInstance().

When in doubt, I tend to mimic what Craft itself is doing. And since Craft uses ::$app, it makes a good case for the ::$plugin convention.

timkelty commented 5 years ago

Consistency might also argue this should be either MyModule::$module, or ::$instance for both Plugin and Module

…but I'll just use getInstance() 🕺

lindseydiloreto commented 5 years ago

I'd love to use MyModule::$module, but Yii already has that reserved.