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.

Files: 
CommentFileSizeAuthor
#43 plugin-2033383-43.patch92.58 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 57,285 pass(es).
[ View ]
#43 interdiff.txt594 bytestim.plunkett
#41 plugin-2033383-41.patch92.82 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 57,298 pass(es).
[ View ]
#41 interdiff.txt19.04 KBtim.plunkett
#39 plugin-2033383-39.patch83.03 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 57,638 pass(es).
[ View ]
#39 interdiff.txt1.18 KBtim.plunkett
#36 plugin-2033383-36.patch81.74 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 57,824 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#36 interdiff.txt2.14 KBtim.plunkett
#33 plugin-2033383-33.patch79.61 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#33 interdiff.txt8.87 KBtim.plunkett
#31 plugin-2033383-31.patch79.74 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 57,218 pass(es).
[ View ]
#31 interdiff.txt6.6 KBtim.plunkett
#26 plugin-2033383-26.patch84.69 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 57,494 pass(es).
[ View ]
#26 interdiff.txt15.89 KBtim.plunkett
#25 interdiff.txt12.8 KBtim.plunkett
#25 plugin-2033383-25.patch74.67 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 57,446 pass(es).
[ View ]
#23 plugin-2033383-23.patch75.82 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 57,049 pass(es), 13 fail(s), and 18 exception(s).
[ View ]
#23 interdiff.txt3.68 KBtim.plunkett
#21 plugin-2033383-21.patch70.67 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#19 plugin-2033383-19.patch69.91 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#14 plugin-2033383-14.patch36 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 57,465 pass(es).
[ View ]
#14 interdiff.txt882 bytestim.plunkett
#12 interdiff.txt1.9 KBtim.plunkett
#12 plugin-2033383-12.patch35.95 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 57,450 pass(es).
[ View ]
#10 plugin-2033383-10.patch34.15 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 56,849 pass(es).
[ View ]
#3 plugin-2033383-3.patch12.04 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 57,162 pass(es).
[ View ]
#3 interdiff.txt2.16 KBtim.plunkett
#1 plugin-2033383-1.patch11.65 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 57,158 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new11.65 KB
FAILED: [[SimpleTest]]: [MySQL] 57,158 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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)

StatusFileSize
new2.16 KB
new12.04 KB
PASSED: [[SimpleTest]]: [MySQL] 57,162 pass(es).
[ View ]

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)

Absolutely. But you have to help name it!

buildConfigForm()
validateConfigForm()
submitConfigForm()

Are what springs to mind.

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)

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.

#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)

It's not there, good idea.

StatusFileSize
new34.15 KB
PASSED: [[SimpleTest]]: [MySQL] 56,849 pass(es).
[ View ]

Renamed, documented, and converted block.

+++ 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

StatusFileSize
new35.95 KB
PASSED: [[SimpleTest]]: [MySQL] 57,450 pass(es).
[ View ]
new1.9 KB

Thanks for the review!

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

Status:Needs work» Needs review
StatusFileSize
new882 bytes
new36 KB
PASSED: [[SimpleTest]]: [MySQL] 57,465 pass(es).
[ View ]

Good catch!

RTBC assuming that comes back green.

Eclipse

Title:Revisit various PluginBags and consider improving APIProvide a default plugin bag for objects with a single plugin

Status:Needs review» Reviewed & tested by the community

As promised in 15

Eclipse

Issue summary:View changes

Updated

Issue summary:View changes

Updated

Title:Provide a default plugin bag for objects with a single pluginProvide 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.

Status:Needs work» Needs review
Issue tags:+Needs issue summary update
StatusFileSize
new69.91 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

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.

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

Whoops.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new3.68 KB
new75.82 KB
FAILED: [[SimpleTest]]: [MySQL] 57,049 pass(es), 13 fail(s), and 18 exception(s).
[ View ]

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new74.67 KB
PASSED: [[SimpleTest]]: [MySQL] 57,446 pass(es).
[ View ]
new12.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.

StatusFileSize
new15.89 KB
new84.69 KB
PASSED: [[SimpleTest]]: [MySQL] 57,494 pass(es).
[ View ]

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

I think it is RTBC as per #17.

Issue summary:View changes

Updated

Updated

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?

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.

StatusFileSize
new6.6 KB
new79.74 KB
PASSED: [[SimpleTest]]: [MySQL] 57,218 pass(es).
[ View ]

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.

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

Status:Needs work» Needs review
StatusFileSize
new8.87 KB
new79.61 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

Thanks! Feels good to move that stuff to Component.

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.

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new2.14 KB
new81.74 KB
FAILED: [[SimpleTest]]: [MySQL] 57,824 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

#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--

Looks good to me :-)

Eclipse

Status:Reviewed & tested by the community» Needs work

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

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new1.18 KB
new83.03 KB
PASSED: [[SimpleTest]]: [MySQL] 57,638 pass(es).
[ View ]

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.

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"

Status:Needs work» Needs review
StatusFileSize
new19.04 KB
new92.82 KB
PASSED: [[SimpleTest]]: [MySQL] 57,298 pass(es).
[ View ]

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.

+++ 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'...

StatusFileSize
new594 bytes
new92.58 KB
PASSED: [[SimpleTest]]: [MySQL] 57,285 pass(es).
[ View ]

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

Status:Needs review» Reviewed & tested by the community

Looks on point to me, let's go!

Eclipse

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.

Issue summary:View changes

Updated

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.

Title:Provide a default plugin bagChange 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!

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

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

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!

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

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

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.

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

PluginFormInterface needs one, and DefaultSinglePluginBag and DefaultPluginBag need one.

Issue summary:View changes

Updated

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.

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

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

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

Status:Active» Needs review

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

Title:Change notice: Provide a default plugin bagProvide 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.

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