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

Files: 
CommentFileSizeAuthor
#35 plugin-2062573-34.patch27.91 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 59,172 pass(es).
[ View ]
#35 interdiff.txt20.84 KBtim.plunkett
#29 interdiff.txt7.12 KBXano
#28 drupal_2062573_28.patch27.81 KBXano
PASSED: [[SimpleTest]]: [MySQL] 59,312 pass(es).
[ View ]
#24 config-2062573-24.patch27.32 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,731 pass(es).
[ View ]
#24 interdiff.txt1.19 KBtim.plunkett
#14 drupal-config-defaults-2062573-14.patch26.56 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#14 interdiff.txt10.33 KBtim.plunkett
#13 drupal-config-defaults-2062573-13.patch17.66 KBclaudiu.cristea
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#11 drupal-config-defaults-2062573-11.patch15.58 KBclaudiu.cristea
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#11 interdiff.txt579 bytesclaudiu.cristea
#9 drupal-config-defaults-2062573-9.patch14.95 KBclaudiu.cristea
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#9 interdiff.txt9.94 KBclaudiu.cristea
#2 drupal-config-defaults-2062573-2.patch9.54 KBclaudiu.cristea
PASSED: [[SimpleTest]]: [MySQL] 57,727 pass(es).
[ View ]

Comments

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

Stupid! Interfaces cannot declare protected methods :)

Changing title and summary accordingly.

Issue summary:View changes

Updated issue summary.

Status:Active» Needs review
Issue tags:+API clean-up
StatusFileSize
new9.54 KB
PASSED: [[SimpleTest]]: [MySQL] 57,727 pass(es).
[ View ]

And the patch.

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?

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

Title:Add method setConfigurationDefaults in ConfigurableImageEffectInterfaceAdd 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.

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? :)

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

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

<?php
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

StatusFileSize
new9.94 KB
new14.95 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new579 bytes
new15.58 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Hmm. Hard to catch

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new17.66 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Adapted BlockBase.

StatusFileSize
new10.33 KB
new26.56 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

Missed the blocks themselves.
Phpstorm++

Great! Looks nice. Anybody reviewing this?

Issue tags:+API change

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

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.

Title:Add method getDefaultConfiguration() in ConfigurableImageEffectBaseAdd method getDefaultConfiguration() in ConfigurablePluginInterface

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.

Status:Needs work» Needs review
StatusFileSize
new1.19 KB
new27.32 KB
PASSED: [[SimpleTest]]: [MySQL] 58,731 pass(es).
[ View ]

Rerolled.

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

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().

Assigned:Unassigned» Xano

Assigned:Xano» Unassigned
Status:Needs work» Needs review
StatusFileSize
new27.81 KB
PASSED: [[SimpleTest]]: [MySQL] 59,312 pass(es).
[ View ]

StatusFileSize
new7.12 KB

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.

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

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

Status:Needs review» Reviewed & tested by the community

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

StatusFileSize
new20.84 KB
new27.91 KB
PASSED: [[SimpleTest]]: [MySQL] 59,172 pass(es).
[ View ]

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

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.

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.

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.

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

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

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

adjust title, tests pass again.

Issue summary:View changes

Updated issue summary.

Title:Add method defaultConfiguration() in ConfigurablePluginInterfaceChange 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.

Assigned:Unassigned» Xano

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

The wrong patch has been committed.

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

Priority:Critical» Major

Setting back to major for the change notice

Assigned:Unassigned» Xano

Assigned:Xano» Unassigned
Priority:Major» Normal
Status:Active» Fixed

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

Thanks!

Great API cleanup.

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

Issue summary:View changes

Updated issue summary.