Problem/Motivation
Drupal has a long long history of "alter"-ism, so basically every info hook has an alter hook as well.
These allowed some advanced use cases if you worked with plugins, like define additional settings depending whether a module exists or not or disable/hide a plugin.
The need for an alter hook came up on porting the views plugins to the PluginApi: #1674356: Make a battleplan for the plugins
Proposed resolution
The idea is to create a decorator which wraps any kind of plugin discovery mechanism and calling a alter hook like hook_plugins_$type_alter.
The class AlterDiscoveryDecorator would take any Discovery as an argument so at the end you will have for example CacheDecorator->AlterDecorator->AnnotatedDiscovery.
Remaining tasks
Write the Decorator
Comment | File | Size | Author |
---|---|---|---|
#28 | 1705702-lost-test-28.patch | 2.14 KB | andypost |
#23 | drupal-1705702-22.patch | 672 bytes | tim.plunkett |
#12 | 1705702-12.patch | 6.33 KB | damiankloip |
#10 | 1705702-alter_decorator-9.patch | 6.23 KB | dawehner |
#9 | 1705702-alter_decorator-8.patch | 0 bytes | dawehner |
Comments
Comment #1
EclipseGc CreditAttribution: EclipseGc commentedAnd remove this functionality from the HookDiscovery.
Comment #2
dawehnerHere is a first version of the patch.
I'm wondering whether the plugin type should be used or as before the hook name itself.
Comment #4
EclipseGc CreditAttribution: EclipseGc commentedAppears to be missing the alter decorator.
Comment #5
dawehnerNote: you should never post patches directly after a transatlantic flight.
Comment #6
EclipseGc CreditAttribution: EclipseGc commentedso, you're generating a hook_$type_alter hook right now, and it needs to be either $hook instead (allowing everyone to customize their actual hook) or it needs to be $owner, $type so that we can never have conflicts in alter hook names. I personally think I prefer the hook, but I'm open to either. going the hook route will likely allow us to straight port some existing calls with very few issues.
Eclipse
Comment #8
dawehner#5: 1705702-alter_decorator-5.patch queued for re-testing.
Comment #9
dawehnerUpdated the patch based on #6 and fixed some doc issues.
Comment #10
dawehner.
Comment #12
damiankloip CreditAttribution: damiankloip commentedFixed the test class module enabling and the $hook property on the AlterDecorator class. This should pass now...
Comment #13
EclipseGc CreditAttribution: EclipseGc commentedI've not yet reviewed it within my own work, but the AlterDecorator looks exactly the way I had imagined, so ++ to this.
Comment #14
neclimdulI'm still fairly negative toward the idea of modifying definitions but I understand there's a long history so... works as expected and is simple enough.
Comment #15
EclipseGc CreditAttribution: EclipseGc commentedActually I was considering how great this could be for swapping out classes on a specific plugin id. I think there is some potential awesome to be had.
Comment #16
sunLet's make sure to tag plugin system related issues accordingly - this issue didn't appear in the list :)
This issue has just been mentioned in #1763974: Convert entity type info into plugins
That said, I'm not sure whether it makes sense. I guess it's the pluginified incarnation of hook_*_info_alter(). And I guess it really depends on what information is ultimately going to be contained in the plugin annotations.
Comment #17
EclipseGc CreditAttribution: EclipseGc commentedWorth noting that this alter is more than just an annotation alteration since we could alter the class key which would allow us to swap out one class for another. So this is more like being able to extend a class and use it in place of the class you extended it from, which is really powerful.
Eclipse
Comment #18
sunThis issue/patch has been noted in #1785256: Widgets as Plugins as a dependency for converting Options widgets to field widget plugins, since those need to alter the widget info.
Comment #19
tim.plunkettViews needs this too (and it's tagged as such).
I know 'major' is mostly symbolic for feature request, but bumping it anyway.
Comment #20
EclipseGc CreditAttribution: EclipseGc commentedI've been actively using this in my new blocks work, and as far as I'm concerned it's RTBC.
Comment #21
webchickMy first thought in seeing this was "Gee, we seem to be replacing one line of code with 163 lines of code." :P~
I asked timplunkett for more details on the Views use case. Contextual links needs to know to add links to page views but not RSS views, for example. To identify views, Views intends to use AnnotatedClassDiscovery, whereas 8.x currently hard-codes alters to HookDiscovery mechanism. Splitting this out into its own decorator means you can use alter functionality on any other type of discovery mechanism. That sounds like nice flexibility.
Committed and pushed to 8.x. Thanks!
Comment #22
tim.plunkettHow did this not fail before?
Comment #23
tim.plunkettEr.
Comment #24
dawehnerI came to the same conclusion ... but really why did this not fail before?
I guess there is a plugin system which is not implemented by any module? drupal_alter probably took care of defining the variable. We all like php, right?
Comment #25
webchickCommitted that to 8.x to un-break testbot.
I asked timplunkett if we needed test coverage for this. He said that since we're unlikely to uninstantiate that variable again, we should be ok. Additionally, the current state where we have a plugin system just hanging out there and not implemented by anything should probably be rare. Makes me a little nervous to go without test coverage, but doing so for now.
Comment #26
andypostYes, first we need to extend out test for annotated plugins #1778942: Discovery::getDefinition() / getDefinitions() : inconsistent return values for "no result"
Comment #27
tstoecklerUnless I'm totally missing something /core/modules/system/lib/Drupal/system/Tests/Plugin/AlterDecoratorTest.php was not committed along with the rest. I did see the second commit for the missing files, but the file above was not part of that.
Comment #28
andypostYep
Comment #29
tstoecklerComment #30
webchickHeh, thanks. :P I'm not sure how I forgot to both git apply --index AND git status, but apparently I was having a bad day. :P
Committed and pushed that to 8.x, too. Yay!