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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

EclipseGc’s picture

And remove this functionality from the HookDiscovery.

dawehner’s picture

Status: Active » Needs review
FileSize
4.07 KB

Here is a first version of the patch.

I'm wondering whether the plugin type should be used or as before the hook name itself.

Status: Needs review » Needs work

The last submitted patch, 1705702-alter_decorator-2.patch, failed testing.

EclipseGc’s picture

Appears to be missing the alter decorator.

dawehner’s picture

Status: Needs work » Needs review
FileSize
6.21 KB

Note: you should never post patches directly after a transatlantic flight.

EclipseGc’s picture

so, 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

Status: Needs review » Needs work
Issue tags: -VDC, -Blocks-Layouts

The last submitted patch, 1705702-alter_decorator-5.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
Issue tags: +VDC, +Blocks-Layouts

#5: 1705702-alter_decorator-5.patch queued for re-testing.

dawehner’s picture

Updated the patch based on #6 and fixed some doc issues.

dawehner’s picture

Status: Needs review » Needs work

The last submitted patch, 1705702-alter_decorator-9.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
6.33 KB

Fixed the test class module enabling and the $hook property on the AlterDecorator class. This should pass now...

EclipseGc’s picture

I've not yet reviewed it within my own work, but the AlterDecorator looks exactly the way I had imagined, so ++ to this.

neclimdul’s picture

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

EclipseGc’s picture

Actually 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.

sun’s picture

Component: other » plugin system
Issue tags: +Plugin system

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

EclipseGc’s picture

Worth 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

sun’s picture

This 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.

tim.plunkett’s picture

Priority: Normal » Major

Views needs this too (and it's tagged as such).
I know 'major' is mostly symbolic for feature request, but bumping it anyway.

EclipseGc’s picture

Status: Needs review » Reviewed & tested by the community

I've been actively using this in my new blocks work, and as far as I'm concerned it's RTBC.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

My 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!

tim.plunkett’s picture

Status: Fixed » Needs review

How did this not fail before?

tim.plunkett’s picture

FileSize
672 bytes

Er.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

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

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed 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.

andypost’s picture

tstoeckler’s picture

Status: Fixed » Needs work

Unless 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.

andypost’s picture

Status: Needs work » Needs review
FileSize
2.14 KB

Yep

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community
webchick’s picture

Status: Reviewed & tested by the community » Fixed

Heh, 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!

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