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.
Related Issues
Comment | File | Size | Author |
---|---|---|---|
#43 | plugin-2033383-43.patch | 92.58 KB | tim.plunkett |
#43 | interdiff.txt | 594 bytes | tim.plunkett |
#41 | plugin-2033383-41.patch | 92.82 KB | tim.plunkett |
#41 | interdiff.txt | 19.04 KB | tim.plunkett |
#39 | plugin-2033383-39.patch | 83.03 KB | tim.plunkett |
Comments
Comment #1
tim.plunkettActually, 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)
Comment #3
tim.plunkettComment #4
yched CreditAttribution: yched commentedCould 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)
Comment #5
tim.plunkettAbsolutely. But you have to help name it!
buildConfigForm()
validateConfigForm()
submitConfigForm()
Are what springs to mind.
Comment #6
yched CreditAttribution: yched commentedIf 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)
Comment #7
tim.plunkettIf 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.
Comment #8
yched CreditAttribution: yched commented#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)
Comment #9
tim.plunkettIt's not there, good idea.
Comment #10
tim.plunkettRenamed, documented, and converted block.
Comment #11
dawehnerI guess the description should describe what this plugin bag does.
Needs also docs.
You could just use String::format
Comment #12
tim.plunkettThanks for the review!
Comment #13
EclipseGc CreditAttribution: EclipseGc commentedNot OK for a Component.
Otherwise this seems pretty sane to me.
Eclipse
Comment #14
tim.plunkettGood catch!
Comment #15
EclipseGc CreditAttribution: EclipseGc commentedRTBC assuming that comes back green.
Eclipse
Comment #16
tim.plunkettOpened #2048879: Provide a default plugin bag for objects with multiple plugins as the companion issue.
Comment #17
EclipseGc CreditAttribution: EclipseGc commentedAs promised in 15
Eclipse
Comment #17.0
EclipseGc CreditAttribution: EclipseGc commentedUpdated
Comment #17.1
tim.plunkettUpdated
Comment #18
tim.plunkettAfter 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.
Comment #19
tim.plunkettOkay, 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.
Comment #21
tim.plunkettWhoops.
Comment #23
tim.plunkettComment #25
tim.plunkettOkay, 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.
Comment #26
tim.plunkettIf this is green I'd be happy with it as-is.
Comment #27
jibranI think it is RTBC as per #17.
Comment #27.0
jibranUpdated
Comment #28
tim.plunkettUpdated
Comment #29
xjmThanks @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
?Comment #30
dawehnerThe 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.
Comment #31
tim.plunkettI 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.
Comment #32
EclipseGc CreditAttribution: EclipseGc commentedShould be in Component
Should be in Component.
move to String::format()?
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
Comment #33
tim.plunkettThanks! Feels good to move that stuff to Component.
Comment #34
EclipseGc CreditAttribution: EclipseGc commentedrtbc pending green
Eclipse
Comment #36
tim.plunkett#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--
Comment #37
EclipseGc CreditAttribution: EclipseGc commentedLooks good to me :-)
Eclipse
Comment #39
tim.plunkettSee, 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.
Comment #40
alexpottThis 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.
I think there is potential for a lot of confusion between
setConfig()
andsetConfiguration()
. 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 thatsetConfig()
should be renamed tosetConfigurationValue()
which works for me.The new key should be "id" and not "plugin"
Comment #41
tim.plunkettI 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.
Comment #42
tim.plunkettI 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'...
Comment #43
tim.plunkettAccidentally left one of my debugging hacks in that last patch.
Comment #44
EclipseGc CreditAttribution: EclipseGc commentedLooks on point to me, let's go!
Eclipse
Comment #45
xjmI 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.
Comment #45.0
xjmUpdated
Comment #46
alexpottDiscussed 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.
Comment #47
alexpottIssue summary has been updated https://drupal.org/node/2033383/revisions/view/2780321/2786933
Committed a2f9a60 and pushed to 8.x. Thanks!
Comment #48
andypostFiled major #2055779: Provide a better fallback for missing filters probably we need more tests for each plugin type
Comment #49
amateescu CreditAttribution: amateescu commentedIs 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?
Comment #50
tim.plunkettSee, how would I have known field module went and did its own thing? :)
At the bottom of Drupal\Component\Plugin\PluginBase, it says:
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!
Comment #51
yched CreditAttribution: yched commented#1764380: Merge PluginSettingsInterface into ConfigurableInterface was the issue to try to generalize it, but I never followed up :-/
Comment #52
tim.plunkettI'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.
Comment #53
catch#2083415: [META] Write up all outstanding change notices before release.
Comment #54
tim.plunketthttps://drupal.org/node/2088589 covers the ConfigurablePluginInterface bit.
PluginFormInterface needs one, and DefaultSinglePluginBag and DefaultPluginBag need one.
Comment #54.0
tim.plunkettUpdated
Comment #55
xjmTagging "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.
Comment #56
tim.plunkettOkay, added https://drupal.org/node/2190815 for PluginFormInterface, working on one for DefaultPluginBag/DefaultSinglePluginBag
Comment #57
tim.plunkettFinal one: https://drupal.org/node/2190891
Comment #58
larowlanIs there an example we can use on https://drupal.org/node/2190815?
https://drupal.org/node/2190891 looks ok
Comment #59
xjmComment #60
tim.plunkettI added an example to https://drupal.org/node/2190815
Comment #61
larowlanThanks
Comment #63
ParisLiakos CreditAttribution: ParisLiakos commentedshouldnt the change records be published now? or there are other issues required for that?