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.

TODO

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

claudiu.cristea’s picture

Title: Add a protected setConfigurationDefaults method in ConfigurableImageEffectInterface » Add method setConfigurationDefaults in ConfigurableImageEffectInterface

Stupid! Interfaces cannot declare protected methods :)

Changing title and summary accordingly.

claudiu.cristea’s picture

Issue summary: View changes

Updated issue summary.

claudiu.cristea’s picture

Status: Active » Needs review
Issue tags: +API clean-up
FileSize
9.54 KB

And the patch.

andypost’s picture

Awesome idea.

@@ -103,10 +103,17 @@ public function setConfiguration(array $configuration) {
+  public function getConfigurationDefaults() {
+    return array();

@@ -54,12 +49,16 @@ public function getSummary() {
+    return parent::getConfigurationDefaults() + array(

@@ -63,6 +56,15 @@ public function getSummary() {
+    return parent::getConfigurationDefaults() + array(

Any reason to make useless call to parent?

claudiu.cristea’s picture

@andypost, Those 2 are not useless. Their parent is not ImageEffectBase but ResizeImageEffect. And ResizeImageEffect::getConfigurationDefaults() provides defaults for width and height, values also used in crop and scale.

tim.plunkett’s picture

Title: Add method setConfigurationDefaults in ConfigurableImageEffectInterface » Add method getDefaultConfiguration() in ConfigurableImageEffectBase

ConfigurableActionBase 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.

claudiu.cristea’s picture

ConfigurableActionBase 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.

I think, yes, we can name it so.

Also you need a new base class, protected methods can't be on interfaces.

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 that getDefaultConfiguration() 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? :)

tim.plunkett’s picture

Why would this method need to be public? Who would call it?

claudiu.cristea’s picture

IRC:

[10:24pm] timplunkett: claudiu_cristea: "protected method cannot be declared on interfaces" is not a reason to make something public
[10:24pm] claudiu_cristea: timplunkett: It doesn't need to be public
[10:24pm] timplunkett: claudiu_cristea: if it is an internal method, then it should be protected
[10:24pm] claudiu_cristea: timplunkett: but I want to force implementations to implement it
[10:25pm] timplunkett: claudiu_cristea: then mark it abstract
[10:25pm] claudiu_cristea: timplunkett: you mean, a new base class instead of interface?
[10:25pm] timplunkett: claudiu_cristea: abstract protected function getDefaultConfiguration();
[10:25pm] timplunkett: claudiu_cristea: well
[10:25pm] timplunkett: claudiu_cristea: we have two options here
[10:25pm] timplunkett: claudiu_cristea: either you add a new base class with the abstract protected method
[10:26pm] timplunkett: claudiu_cristea: or we make it public in actions as well and just put it on ConfigurablePluginInterface and be done with it
[10:26pm] claudiu_cristea: timplunkett: It was an option, I went to interface because it's more simple
[10:27pm] timplunkett: claudiu_cristea: we should be consistent across the plugins. i think putting it on ConfigurablePluginInterface might be best actually, even if not perfect
[10:27pm] claudiu_cristea: timplunkett: the 2nd option is to add ConfigurablePluginInterface as the 2nd interface?
[10:28pm] timplunkett: claudiu_cristea: yeah, ConfigurableImageEffectInterface extends ConfigurablePluginInterface, ImageEffectInterface makes sense to me
[10:29pm] claudiu_cristea: timplunkett: and switch add the existing method to interface as well
[10:29pm] timplunkett: claudiu_cristea: yes

Option 1

Implement a new base class for configurable effects and add there a protected method getDefaultConfiguration()

Option 2

interface ConfigurableImageEffectInterface extends ConfigurablePluginInterface, ImageEffectInterface {
}

And add getDefaultConfiguration() in ConfigurablePluginInterface. Make all existing implementations public.

My findings:

On option 1:

  1. This seems "cleaner"
  2. Doesn't make any unneeded change from protected to public.
  3. It's more complex because adds a new class in hierarchy.

On option 2:

  1. Now ConfigurableImageEffectInterface extends ImageEffectInterface which already extends ConfigurablePluginInterface. So, I think that's enough and nothing need to be done except changing existing implementations to public.
  2. We switch methods from protected to public without needing that (as functionality). So I don't see why this is this better than #2?

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

claudiu.cristea’s picture

OK. Followed option 2. Let's see tests.

Status: Needs review » Needs work

The last submitted patch, drupal-config-defaults-2062573-9.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
579 bytes
15.58 KB

Hmm. Hard to catch

Status: Needs review » Needs work

The last submitted patch, drupal-config-defaults-2062573-11.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
17.66 KB

Adapted BlockBase.

tim.plunkett’s picture

Missed the blocks themselves.
Phpstorm++

claudiu.cristea’s picture

Great! Looks nice. Anybody reviewing this?

dawehner’s picture

Issue tags: +API change

This looks fine in general, beside the problem that this is an API change for blocks ...

dawehner’s picture

tim.plunkett’s picture

claudiu.cristea’s picture

tim.plunkett’s picture

I 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.

tim.plunkett’s picture

Title: Add method getDefaultConfiguration() in ConfigurableImageEffectBase » Add method getDefaultConfiguration() in ConfigurablePluginInterface
tim.plunkett’s picture

Issue tags: -API change, -API clean-up

Status: Needs review » Needs work
Issue tags: +API change, +API clean-up

The last submitted patch, drupal-config-defaults-2062573-14.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
1.19 KB
27.32 KB

Rerolled.

tim.plunkett’s picture

#24: config-2062573-24.patch queued for re-testing.

pwolanin’s picture

Status: Needs review » Needs work

nearly RTBC, but there are a number of places where the doxygen needs to be fixed since it still says:

* Overrides \Drupal\block\BlockBase::settings().
Xano’s picture

Assigned: Unassigned » Xano
Xano’s picture

Assigned: Xano » Unassigned
Status: Needs work » Needs review
FileSize
27.81 KB
Xano’s picture

FileSize
7.12 KB
pwolanin’s picture

Status: Needs review » Reviewed & tested by the community

Looks 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.

Status: Reviewed & tested by the community » Needs work
Issue tags: -API change, -API clean-up

The last submitted patch, drupal_2062573_28.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
Issue tags: +API change, +API clean-up

#28: drupal_2062573_28.patch queued for re-testing.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Drupal\system\Tests\System\AccessDeniedTest and friends random fail.

tim.plunkett’s picture

FileSize
20.84 KB
27.91 KB

Per webchick and msonnabaum's request, renaming all of getDefaultConfiguration to defaultConfiguration

webchick’s picture

Status: Reviewed & tested by the community » Needs work

Talked 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.

webchick’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: +Approved API change

Sorry. Cross-post. Also tagging as an Approved API change.

Status: Reviewed & tested by the community » Needs work
Issue tags: -API change, -API clean-up, -Approved API change

The last submitted patch, plugin-2062573-34.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review

#35: plugin-2062573-34.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, plugin-2062573-34.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
Issue tags: +API change, +API clean-up

#35: plugin-2062573-34.patch queued for re-testing.

pwolanin’s picture

Title: Add method getDefaultConfiguration() in ConfigurablePluginInterface » Add method defaultConfiguration() in ConfigurablePluginInterface
Status: Needs review » Reviewed & tested by the community

adjust title, tests pass again.

pwolanin’s picture

Issue summary: View changes

Updated issue summary.

webchick’s picture

Title: Add method defaultConfiguration() in ConfigurablePluginInterface » Change notice: Add method defaultConfiguration() in ConfigurablePluginInterface
Priority: Normal » Major
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record, +Approved API change

Awesome, thanks.

Committed and pushed to 8.x.

We'll need a change notice for this.

Xano’s picture

Assigned: Unassigned » Xano
Xano’s picture

Component: image.module » plugin system
Assigned: Xano » Unassigned
Priority: Major » Critical

The wrong patch has been committed.

alexpott’s picture

Reverted the original commit and committed c435ef8 and pushed to 8.x. Thanks!

alexpott’s picture

Priority: Critical » Major

Setting back to major for the change notice

Xano’s picture

Assigned: Unassigned » Xano
Xano’s picture

Assigned: Xano » Unassigned
Priority: Major » Normal
Status: Active » Fixed
tim.plunkett’s picture

Title: Change notice: Add method defaultConfiguration() in ConfigurablePluginInterface » Add method defaultConfiguration() in ConfigurablePluginInterface
Issue tags: -Needs change record +Plugin system

Thanks!

claudiu.cristea’s picture

Great API cleanup.

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary: View changes

Updated issue summary.