Updated: Comment #16

Problem/Motivation

BlockPluginBag and ActionBag share the same architecture, and contrib will reuse this many times as well.
In the same vein as DefaultPluginManager, we need a sane default.

In addition, Action introduced a useful interface but kept it within its own namespace. This abstracts it.

Proposed resolution

(Description of the proposed solution, the rationale behind it, and workarounds for people who cannot use the patch.)

Remaining tasks

N/A

User interface changes

N/A

API changes

\Drupal\Core\Action\ConfigurableActionInterface was split into two classes:
\Drupal\Core\Plugin\PluginFormInterface which represents what plugins needed out of #2006248: Add an EmbeddableFormInterface so that FormInterface can delegate to multiple objects
\Drupal\Core\Plugin\ConfigurablePluginInterface which gives a standard way to retrieve configuration (See the comment at the bottom of \Drupal\Component\Plugin\PluginBase)

\Drupal\block\BlockPluginInterface no longer has form(), validate(), submit(), getConfig(), or setConfig(), but instead PluginFormInterface::buildConfigurationForm(), PluginFormInterface::validateConfigurationForm(), PluginFormInterface::submitConfigurationForm(), ConfigurablePluginInterface::getConfiguration(), and BlockPluginInterface::setConfigurationValue().

\Drupal\filter\Plugin\FilterInterface no longer has setPluginConfiguration(), replace by ConfigurablePluginInterface::setConfiguration()

\Drupal\Core\Plugin\DefaultSinglePluginBag and \Drupal\Core\Plugin\DefaultPluginBag are added as base classes for the two approaches to plugin bags.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

Status: Active » Needs review
FileSize
11.65 KB

Actually, I think Action has the most generic stuff.
I've reused ActionBag and ConfigurableActionInterface here: #2042807: Convert search plugins to use a ConfigEntity and a PluginBag

This is the relevant part of that.

We still need a pattern for multiple plugins (views displays, filters, image effects, tips)

tim.plunkett’s picture

FileSize
2.16 KB
12.04 KB
yched’s picture

Could we rename ConfigurablePluginInterface::form() to something more specific ?
This feels like a base class squatting a generic method name, preventing its use by domain classes / interfaces for which it would make sense. ConfigurablePluginInterface looks like something we'd want to use for widgets, but WidgetInterface already has a form() method, because that's what widgets do :-)

In short, base classes should preferably "namespace" their method names with the specific functionality they provide, so as not to constrain actual domain classes extending them (and/or avoid clashes with other base classes / interfaces that domain classes might want to extend/implement as well)

tim.plunkett’s picture

Absolutely. But you have to help name it!

buildConfigForm()
validateConfigForm()
submitConfigForm()

Are what springs to mind.

yched’s picture

If the interface is named ConfigurablePluginInterface, then IMO we should go accordingly with full "Configuration" in the method name. Other than that, #5 seems fine.

Other possibility:
Widgets and formatters use a PluginWithSettings interface (provided by field module, I had an issue to generalize it to other plugins, but this stalled so far), with a settingsForm() method.

Also, a submit() method makes me tickle a bit, because typically it shouldn't be the job of a plugin to save its own settings. A field formatter can be used as part of an EntityDisplay or a View, and its settings will be embedded in.the CMI file of either of those configEntities (and contrib can totally invent other uses)

tim.plunkett’s picture

If you look at how that submit() is used:
\Drupal\action\Plugin\Action\GotoAction::submit()

$this->configuration['url'] = $form_state['values']['url'];

You just need to process the form state values and store it on yourself, something else (the ConfigEntity in this case) will actually do the storage for you.

yched’s picture

#7: true - should this be made clear in the phpdoc for the method then ?
(Maybe it already is, hard for me to check actual code right now)

tim.plunkett’s picture

It's not there, good idea.

tim.plunkett’s picture

FileSize
34.15 KB

Renamed, documented, and converted block.

dawehner’s picture

+++ b/core/lib/Drupal/Core/Plugin/DefaultSinglePluginBag.phpundefined
@@ -2,18 +2,18 @@
+ * Provides a default plugin bag for a plugin type.

I guess the description should describe what this plugin bag does.

+++ b/core/lib/Drupal/Core/Plugin/DefaultSinglePluginBag.phpundefined
@@ -23,7 +23,12 @@ class ActionBag extends PluginBag {
   /**
-   * Constructs a new ActionBag object.
+   * @var array
+   */
+  protected $configuration;
+

Needs also docs.

+++ b/core/modules/block/lib/Drupal/block/BlockPluginBag.phpundefined
@@ -46,18 +37,14 @@ public function __construct(PluginManagerInterface $manager, array $instance_ids
+      throw new PluginException(format_string("The block '@block' did not specify a plugin.", array('@block' => $this->blockId)));

You could just use String::format

tim.plunkett’s picture

FileSize
35.95 KB
1.9 KB

Thanks for the review!

EclipseGc’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/Plugin/DefaultSinglePluginBag.phpundefined
@@ -0,0 +1,63 @@
+    $this->instanceIDs = drupal_map_assoc($instance_ids);

Not OK for a Component.

Otherwise this seems pretty sane to me.

Eclipse

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
882 bytes
36 KB

Good catch!

EclipseGc’s picture

RTBC assuming that comes back green.

Eclipse

tim.plunkett’s picture

Title: Revisit various PluginBags and consider improving API » Provide a default plugin bag for objects with a single plugin
EclipseGc’s picture

Status: Needs review » Reviewed & tested by the community

As promised in 15

Eclipse

EclipseGc’s picture

Issue summary: View changes

Updated

tim.plunkett’s picture

Issue summary: View changes

Updated

tim.plunkett’s picture

Title: Provide a default plugin bag for objects with a single plugin » Provide a default plugin bag
Status: Reviewed & tested by the community » Needs work

After further discussion with @EclipseGc, I'm merging #2048879: Provide a default plugin bag for objects with multiple plugins in with this one. Otherwise we'd need to effectively rewrite this.

tim.plunkett’s picture

Status: Needs work » Needs review
Issue tags: +Needs issue summary update
FileSize
69.91 KB

Okay, this is much more what I had in mind!

And of course, the one time I write an issue summary, it gets out of date.

tim.plunkett’s picture

FileSize
70.67 KB

Whoops.

Status: Needs review » Needs work

The last submitted patch, plugin-2033383-21.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
3.68 KB
75.82 KB

Status: Needs review » Needs work

The last submitted patch, plugin-2033383-23.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
74.67 KB
12.8 KB

Okay, I had to back out some of the changes to FilterBag. The interdiff looks like I'm adding a bunch of weird stuff, but its just restoring code that was in HEAD.

tim.plunkett’s picture

FileSize
15.89 KB
84.69 KB

If this is green I'd be happy with it as-is.

jibran’s picture

I think it is RTBC as per #17.

jibran’s picture

Issue summary: View changes

Updated

tim.plunkett’s picture

Updated

xjm’s picture

Issue tags: +API change

Thanks @tim.plunkett for starting to document the API changes, though it looks like the list might not be quite complete. From #4, the method renames for blocks are a required part of this, and it should be fairly low-impact since they're mostly only used by the block API so far. This might affect Princess; has @sdboyer had a chance to look at it?

I'm assuming the added filter IDs are necessary because of using the new base class? But what's with the changes around ViewStorageInterface?

dawehner’s picture

The change in the ViewStorageInterface is kind of out of scope of this issue, as it just moves the method which exits on the view object and pull it up to the interface. Remember ViewStorageInterface probably should be called ViewInterface one day, as people confuse it with the storage controller.

tim.plunkett’s picture

FileSize
6.6 KB
79.74 KB

I had made a bunch of other changes to ViewStorageInterface that I finally reverted, but I left these in. Reverted.
I also decided not to delete ActionsBag and TipsBag, in case we ever need to make improvements. They're just empty now.

EclipseGc’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/Plugin/ConfigurablePluginInterface.phpundefined
@@ -0,0 +1,31 @@
+ * Contains \Drupal\Core\Plugin\ConfigurablePluginInterface.

Should be in Component

+++ b/core/lib/Drupal/Core/Plugin/DefaultPluginBag.phpundefined
@@ -0,0 +1,139 @@
+ * Contains \Drupal\Core\Plugin\DefaultPluginBag.

Should be in Component.

+++ b/core/lib/Drupal/Core/Plugin/DefaultPluginBag.phpundefined
@@ -0,0 +1,139 @@
+        throw new PluginException(format_string("Unknown plugin ID '@instance'.", array('@instance' => $instance_id)));

move to String::format()?

+++ b/core/lib/Drupal/Core/Plugin/DefaultSinglePluginBag.phpundefined
@@ -0,0 +1,64 @@
+ * Contains \Drupal\Core\Plugin\DefaultSinglePluginBag.

Should be in Component

This seems pretty kosher to me. Let's just move this stuff around a little and use String::format for our strings. I'd happily rtbc it after we do that pass.

Eclipse

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
8.87 KB
79.61 KB

Thanks! Feels good to move that stuff to Component.

EclipseGc’s picture

Status: Needs review » Reviewed & tested by the community

rtbc pending green

Eclipse

Status: Reviewed & tested by the community » Needs work

The last submitted patch, plugin-2033383-33.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
2.14 KB
81.74 KB

#1957346: Add some settings on the block display to allow overrides on the block instance configuration added two calls to a renamed method.
phpunit++, qa.d.o running them through simpletest--

EclipseGc’s picture

Looks good to me :-)

Eclipse

Status: Reviewed & tested by the community » Needs work

The last submitted patch, plugin-2033383-36.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
1.18 KB
83.03 KB

See, THAT is why the simpletest phpunit runner is terrible. It only gives you PHP Fatal for the unit tests, and masks all of the results of the WebTests.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

This change is lovely... gets rid of loads of duplicate code and unifies the way plugin bags work. This is going to make it much easier for contrib.

+++ b/core/modules/block/lib/Drupal/block/BlockBase.phpundefined
@@ -50,50 +50,28 @@ public function settings() {
+  public function setConfiguration(array $configuration) {
+    $this->configuration = $configuration;
+  }
+
+  /**
+   * {@inheritdoc}
    */
   public function setConfig($key, $value) {
     $this->configuration[$key] = $value;
   }

I think there is potential for a lot of confusion between setConfig() and setConfiguration(). This would be a critical followup which we are no longer supposed to have so I think we should resolve this here. In IRC both @timplunkett and @EclipseGC have said that setConfig() should be renamed to setConfigurationValue() which works for me.

+++ b/core/modules/filter/tests/filter_test/config/filter.format.filter_test.ymlundefined
@@ -9,16 +9,19 @@ status: '1'
 langcode: en
 filters:
   filter_html_escape:
+    plugin: filter_html_escape
     module: filter
     status: '1'
     weight: '-10'
     settings: {  }
   filter_autop:
+    plugin: filter_autop
     module: filter
     status: '1'
     weight: '0'
     settings: {  }
   filter_url:
+    plugin: filter_url
     module: filter
     status: '1'
     weight: '0'

The new key should be "id" and not "plugin"

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
19.04 KB
92.82 KB

I was teaching myself PHPUnit while writing tests for DisplayBag, FilterBag, and ActionBag (I'm going to leave that for a separate issue because my tests will need some help).

I found some bizarre things, and added some nonintrusive nice-to-have docsblocks.

Also made the fixes @alexpott pointed out, thanks!

Interdiff excluding whitespace.

tim.plunkett’s picture

+++ b/core/lib/Drupal/Component/Plugin/DefaultPluginBag.phpundefined
@@ -41,7 +41,7 @@ class DefaultPluginBag extends PluginBag {
-  protected $pluginKey = 'plugin';
+  protected $pluginKey = 'id';

I also made this change because 'id' is much more common. If it wasn't the end of July, I'd switch everything over to using 'id'...

tim.plunkett’s picture

FileSize
594 bytes
92.58 KB

Accidentally left one of my debugging hacks in that last patch.

EclipseGc’s picture

Status: Needs review » Reviewed & tested by the community

Looks on point to me, let's go!

Eclipse

xjm’s picture

I also think the API change list in the summary is still not quite complete? At least, it looks like it stops mid-sentence. :) And it also includes at least one change that was (rightly I think) reverted.

xjm’s picture

Issue summary: View changes

Updated

alexpott’s picture

Issue tags: +Approved API change

Discussed with @catch and we both feel that brings order to a wild-west and makes it much easier for contrib. So lets update the issue summary as per #45 and get this in.

alexpott’s picture

Title: Provide a default plugin bag » Change notice: Provide a default plugin bag
Priority: Normal » Critical
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

Issue summary has been updated https://drupal.org/node/2033383/revisions/view/2780321/2786933

Committed a2f9a60 and pushed to 8.x. Thanks!

andypost’s picture

Filed major #2055779: Provide a better fallback for missing filters probably we need more tests for each plugin type

amateescu’s picture

+++ b/core/lib/Drupal/Component/Plugin/ConfigurablePluginInterface.php
@@ -0,0 +1,31 @@
+/**
+ * Provides an interface for a configurable plugin.
+ */
+interface ConfigurablePluginInterface {

Is this what I think it is.. finally a semi-formalization of Drupal\field\Plugin\PluginSettingsInterface that field.module needed/introduced almost one year ago? :)

Should we try to explore adding the "missing" methods to the new ConfigurablePluginInterface and switching field plugins to it in a followup?

tim.plunkett’s picture

See, how would I have known field module went and did its own thing? :)

At the bottom of Drupal\Component\Plugin\PluginBase, it says:

// Note: Plugin configuration is optional so its left to the plugin type to
// require a getter as part of its interface.

ConfigurationPluginInterface was meant to fill that need.

I know blocks also have a getDefaultConfiguration() method, as well as a setConfigurationValue(), which maps to PluginSettingsInterface's getDefaultSettings() and setSetting(). I'm not sure if they ALL belong on the interface...

But yes! A follow-up!

yched’s picture

See, how would I have known field module went and did its own thing? :)

#1764380: Merge PluginSettingsInterface into ConfigurableInterface was the issue to try to generalize it, but I never followed up :-/

tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned

I'm told that no one will volunteer to write a change notice if the issue is assigned, and there's a sprint coming up.
Feel free to ping me if any questions arise.

catch’s picture

tim.plunkett’s picture

https://drupal.org/node/2088589 covers the ConfigurablePluginInterface bit.

PluginFormInterface needs one, and DefaultSinglePluginBag and DefaultPluginBag need one.

tim.plunkett’s picture

Issue summary: View changes

Updated

xjm’s picture

Issue tags: +Missing change record

Tagging "Missing change record", as completing the change record updates here blocks a beta: #2083415: [META] Write up all outstanding change notices before release.

The patch for this issue was committed on August 1, 2013, meaning that the change record for this issue has been missing for nearly six months.

tim.plunkett’s picture

Okay, added https://drupal.org/node/2190815 for PluginFormInterface, working on one for DefaultPluginBag/DefaultSinglePluginBag

tim.plunkett’s picture

larowlan’s picture

Is there an example we can use on https://drupal.org/node/2190815?

https://drupal.org/node/2190891 looks ok

xjm’s picture

Status: Active » Needs review
tim.plunkett’s picture

I added an example to https://drupal.org/node/2190815

larowlan’s picture

Title: Change notice: Provide a default plugin bag » Provide a default plugin bag
Status: Needs review » Fixed
Issue tags: -Needs change record, -Missing change record

Thanks

Status: Fixed » Closed (fixed)

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

ParisLiakos’s picture

shouldnt the change records be published now? or there are other issues required for that?