Suggested commit message:
Allow plugins to declare themselves a derivative #2184951 by not_chx
please do not credit chx, wrong browser for the reroll.
Problem/Motivation
The plugin derivative system works well in most cases: if you want to display a menu block, one class can handle all the menus. One menu is like the next.
For migration destination plugins this, however, does not work because it needs to work with various entity types and one entity type is definitely not like the other. There needs to derivatives for every entity type but determining which class serves which type is slightly problematic. Some entity types have specific classes, most, however can be served by the content or the config base, depending. However, when defining the derivatives discovering the specific classes is a problem.
Proposed resolution
A small change in the DerivativeDiscoveryDecorator
allows plugins to declare themselves as derivatives. In other words @PluginID('entity:comment)
. Instead of solving the problem of the DerivativeDiscoveryDecorator finding EntityComment, we rely on the annotated discovery already finding it. In theory this was agreed on by dawehner
I agree that a plugin should be able to specify a plugin ID which is not overridden by a derivative class
Remaining tasks
None.
User interface changes
None.
API changes
None. This is an addition.
Major as it's a migrate blocker.
Comment | File | Size | Author |
---|---|---|---|
#28 | 2184951_28.patch | 9.08 KB | chx |
#28 | diffdiff.txt | 461 bytes | chx |
Comments
Comment #1
not_chx CreditAttribution: not_chx commentedComment #2
not_chx CreditAttribution: not_chx commentedComment #3
not_chx CreditAttribution: not_chx commentedMore straightforward patch.
Comment #4
not_chx CreditAttribution: not_chx commentedWith more comments and more readable code flow than the original (which conflated plugin_definition and base_plugin_definition)
Comment #5
not_chx CreditAttribution: not_chx commentedComment #6
not_chx CreditAttribution: not_chx commentedComment #8
not_chx CreditAttribution: not_chx commentedComment #9
not_chx CreditAttribution: not_chx commentedComment #10
not_chx CreditAttribution: not_chx commentedComment #11
not_chx CreditAttribution: not_chx commentedComment #12
dawehnerIt would have been kinda cool if the other instances of the pattern (as you know views) would have been converted
so we see it also works in "real life" examples, though this does not block this issue to be RTBC.
Comment #13
not_chx CreditAttribution: not_chx commentedOh sure, let's convert Views!
Comment #14
not_chx CreditAttribution: not_chx commentedI will file a followup instead -- turns out my conversion misses all the keys ViewsEntityRow::getDerivativeDefinitions does.
Comment #16
not_chx CreditAttribution: not_chx commented#11 is still RTBC.
Comment #17
tim.plunkettThis seems to be only useful for @PluginId, which is not many things. If we can't get that Views usecase working, I'm not sure it should go in alone...
Comment #18
not_chx CreditAttribution: not_chx commentedOh we absolutely can we make it work, it's just more work -- I need to copy every key and I only copied one.
Comment #19
not_chx CreditAttribution: not_chx commentedHere's all three. I ran the node RowPluginTest and it passed.
Comment #21
not_chx CreditAttribution: not_chx commentedHere's a version that uses derivative settings set by the deriver as default -- ie the plugin definition merely overrides much like the alter did.
Comment #22
dawehnerinterdiff *sniff* *sniff* Note: if you have a couple of minutes to review a patch during breakfast you don't wanna start from 0 again
Comment #23
dawehnerBeside from that it is looking great.
Comment #24
not_chx CreditAttribution: not_chx commentedSorry but the interdiff is 9kb and I doubt that would've helped reviewing a 9kb patch...
Comment #25
dawehnerThank you!
Comment #26
tim.plunkettYes please! Keeping all of the metadata in the file instead of in a procedural alter is awesome.
RTBC +1
Comment #27
not_chx CreditAttribution: not_chx commentedNote that a change notice IMO is not necessary -- this is an addition; the old alter hook still works if anyone happened to use it and there is no change notice talking about defining derivatives anyways. Also, this is such an edge case that I seriously doubt anyone already uses it. Consider how similar Views and migrate are in breadth: they need to cover for every module in core, they have "dynamic" plugin managers (multiple types in one class) and so on -- these are really atypical of a contrib and even more so of a custom module. I do not expect the change in this issue to be used much outside of these two modules.
There's a handbook page https://drupal.org/node/1653226 or two https://drupal.org/node/1653532 which is relevant but they are hopelessly outdated and need to rewritten from ground up anyways.
Comment #28
chx CreditAttribution: chx commentedKeeping up with HEAD. interdiff would be meaningless so I attached a diff of the diffs: the patch didn't change at all except in context and an empty line removal.
Comment #29
not_chx CreditAttribution: not_chx commentedComment #30
not_chx CreditAttribution: not_chx commentedBack to RTBC, no changes, was NR only for bot.
Comment #32
webchick28: 2184951_28.patch queued for re-testing.
Comment #35
chx CreditAttribution: chx commented28: 2184951_28.patch queued for re-testing.
Comment #36
not_chx CreditAttribution: not_chx commentedComment #37
webchickYeah, the code snippet in #26 indeed looks much nicer. The work done in getDefinition() also makes that function more readable as a nice bonus.
Committed and pushed to 8.x. Thanks!
Comment #39
damiankloip CreditAttribution: damiankloip commentedJust opened #2221037: Empty base plugin definitions overwrite derivative definitions when a plugin (annotation/id) is used as a derivative, so referencing here.