Updated: Comment #44
Problem/Motivation
Configurable plugins have variable plublic and protected methods for fetching default configuration. A significant number have a method for this called settings(). Basically, it's because that's a lie, and not actually what it's providing - it's providing default configuration.
Proposed resolution
Add method defaultConfiguration() in ConfigurablePluginInterface
Having "DefaultConfiguration" in the name is much more descriptive and we're already using that convention in a few places, so this just unifies everything under one name.
Note on the naming: "getFooBar" is a convention for a simple function that just deals with an internal object property (e.g. $this->fooBar). In the case where we're doing something fancier than that (like currentUser()) we should drop the "get" prefix so people don't get confused. Or so is the thinking anyway.
Remaining tasks
Add documentation to plugin interfaces by overriding the method.
Consider updating plugins with a protected method that combines the defaults with the passed in configuration rather than doing this logic in the constructor.
User interface changes
No changes in UI.
API changes
New method defultConfiguration()
in ConfigurablePluginInterface
interface. All configurable plugins must implement this method and return an associative array with effect configuration defaults.
In addition, all plugin interfaces extending ComfigurablePluginInterface are expected to override it for the purpose of providing specific documentation about the possible configuration keys and values appropriate for that plugin type.
Related Issues
TODO
Comment | File | Size | Author |
---|---|---|---|
#35 | plugin-2062573-34.patch | 27.91 KB | tim.plunkett |
#35 | interdiff.txt | 20.84 KB | tim.plunkett |
#29 | interdiff.txt | 7.12 KB | Xano |
#28 | drupal_2062573_28.patch | 27.81 KB | Xano |
#24 | config-2062573-24.patch | 27.32 KB | tim.plunkett |
Comments
Comment #1
claudiu.cristeaStupid! Interfaces cannot declare protected methods :)
Changing title and summary accordingly.
Comment #1.0
claudiu.cristeaUpdated issue summary.
Comment #2
claudiu.cristeaAnd the patch.
Comment #3
andypostAwesome idea.
Any reason to make useless call to parent?
Comment #4
claudiu.cristea@andypost, Those 2 are not useless. Their parent is not
ImageEffectBase
butResizeImageEffect
. AndResizeImageEffect::getConfigurationDefaults()
provides defaults for width and height, values also used in crop and scale.Comment #5
tim.plunkettConfigurableActionBase has
protected function getDefaultConfiguration()
, can we reuse that name? Without traits we can't share that code, but we can at least share the naming.Also you need a new base class, protected methods can't be on interfaces.
Comment #6
claudiu.cristeaI think, yes, we can name it so.
Hmm.. I don't get it :) My new method is public. I was mistakenly consider it protected first but changed in the next minute,s realizing that protected method cannot be declared on interfaces. And as long as
ConfigurableActionBase
is on different class hierarchy we'll not extend thatgetDefaultConfiguration()
protected method. That can remain protected and this new one public. They are not the same thing, it's only a unified naming convention.Or, am I missing something? :)
Comment #7
tim.plunkettWhy would this method need to be public? Who would call it?
Comment #8
claudiu.cristeaIRC:
Option 1
Implement a new base class for configurable effects and add there a protected method
getDefaultConfiguration()
Option 2
And add
getDefaultConfiguration()
inConfigurablePluginInterface
. Make all existing implementations public.My findings:
On option 1:
On option 2:
ConfigurableImageEffectInterface
extendsImageEffectInterface
which already extendsConfigurablePluginInterface
. So, I think that's enough and nothing need to be done except changing existing implementations to public.What else?
Trying to have a not-subjective opinion I still like #2 because it's simpler, cleaner. But I'm OK too with one of the 2 suggested by @tim.plunkett
Comment #9
claudiu.cristeaOK. Followed option 2. Let's see tests.
Comment #11
claudiu.cristeaHmm. Hard to catch
Comment #13
claudiu.cristeaAdapted BlockBase.
Comment #14
tim.plunkettMissed the blocks themselves.
Phpstorm++
Comment #15
claudiu.cristeaGreat! Looks nice. Anybody reviewing this?
Comment #16
dawehnerThis looks fine in general, beside the problem that this is an API change for blocks ...
Comment #17
dawehnerWe could also switch to https://api.drupal.org/api/drupal/core%21modules%21field%21lib%21Drupal%...
Comment #18
tim.plunkettSee #1764380: Merge PluginSettingsInterface into ConfigurableInterface
Comment #19
claudiu.cristeaRead #1764380: Merge PluginSettingsInterface into ConfigurableInterface. What's the conclusion?
Comment #20
tim.plunkettI was responding more to @dawehner.
I think we should do this. We currently have 3 ways of doing the same thing, while the rest of the interface is unified.
Comment #21
tim.plunkettComment #22
tim.plunkett#14: drupal-config-defaults-2062573-14.patch queued for re-testing.
Comment #24
tim.plunkettRerolled.
Comment #25
tim.plunkett#24: config-2062573-24.patch queued for re-testing.
Comment #26
pwolanin CreditAttribution: pwolanin commentednearly RTBC, but there are a number of places where the doxygen needs to be fixed since it still says:
Comment #27
XanoComment #28
XanoComment #29
XanoComment #30
pwolanin CreditAttribution: pwolanin commentedLooks good - this will really help us move forward with documenting plugins by overriding these methods on each plugin interface to documented the plugin's specific configuration keys and values.
Comment #32
tim.plunkett#28: drupal_2062573_28.patch queued for re-testing.
Comment #33
tim.plunkettDrupal\system\Tests\System\AccessDeniedTest and friends random fail.
Comment #35
tim.plunkettPer webchick and msonnabaum's request, renaming all of getDefaultConfiguration to defaultConfiguration
Comment #37
webchickTalked about this on IRC w/ pwolanin, msonnabaum, and timplunkett.
I asked originally why we didn't just keep this as settings() so we don't break APIs here. Basically, it's because that's a lie, and not actually what it's providing. Having "DefaultConfiguration" in the name is much more descriptive and we're already using that convention in a few places, so this just unifies everything under one name.
Then, we also bikeshedded the name a bit. "getFooBar" is a convention for a simple function that just deals with an internal object property (e.g. $this->fooBar). In the case where we're doing something fancier than that (like currentUser()) we should drop the "get" prefix so people don't get confused. Or so is the thinking anyway.
Needs work for that small language tweak, and then this is GTG.
Comment #38
webchickSorry. Cross-post. Also tagging as an Approved API change.
Comment #41
tim.plunkett#35: plugin-2062573-34.patch queued for re-testing.
Comment #43
tim.plunkett#35: plugin-2062573-34.patch queued for re-testing.
Comment #44
pwolanin CreditAttribution: pwolanin commentedadjust title, tests pass again.
Comment #44.0
pwolanin CreditAttribution: pwolanin commentedUpdated issue summary.
Comment #45
webchickAwesome, thanks.
Committed and pushed to 8.x.
We'll need a change notice for this.
Comment #46
XanoComment #47
XanoThe wrong patch has been committed.
Comment #48
alexpottReverted the original commit and committed c435ef8 and pushed to 8.x. Thanks!
Comment #49
alexpottSetting back to major for the change notice
Comment #50
XanoComment #51
XanoSee https://drupal.org/node/2088589.
Comment #52
tim.plunkettThanks!
Comment #53
claudiu.cristeaGreat API cleanup.
Comment #54.0
(not verified) CreditAttribution: commentedUpdated issue summary.