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

Files: 
CommentFileSizeAuthor
#28 1705702-lost-test-28.patch2.14 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 42,100 pass(es).
[ View ]
#23 drupal-1705702-22.patch672 bytestim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-1705702-22.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#12 1705702-12.patch6.33 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 39,992 pass(es).
[ View ]
#10 1705702-alter_decorator-9.patch6.23 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 39,985 pass(es), 2 fail(s), and 2 exception(s).
[ View ]
#9 1705702-alter_decorator-8.patch0 bytesdawehner
PASSED: [[SimpleTest]]: [MySQL] 39,976 pass(es).
[ View ]
#5 1705702-alter_decorator-5.patch6.21 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 39,749 pass(es).
[ View ]
#2 1705702-alter_decorator-2.patch4.07 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

Comments

And remove this functionality from the HookDiscovery.

Status:Active» Needs review
StatusFileSize
new4.07 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

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.

Appears to be missing the alter decorator.

Status:Needs work» Needs review
StatusFileSize
new6.21 KB
PASSED: [[SimpleTest]]: [MySQL] 39,749 pass(es).
[ View ]

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

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.

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

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

StatusFileSize
new0 bytes
PASSED: [[SimpleTest]]: [MySQL] 39,976 pass(es).
[ View ]

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

StatusFileSize
new6.23 KB
FAILED: [[SimpleTest]]: [MySQL] 39,985 pass(es), 2 fail(s), and 2 exception(s).
[ View ]

.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new6.33 KB
PASSED: [[SimpleTest]]: [MySQL] 39,992 pass(es).
[ View ]

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

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

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.

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.

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.

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

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.

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.

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.

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!

Status:Fixed» Needs review

How did this not fail before?

StatusFileSize
new672 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-1705702-22.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Er.

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?

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.

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.

Status:Needs work» Needs review
StatusFileSize
new2.14 KB
PASSED: [[SimpleTest]]: [MySQL] 42,100 pass(es).
[ View ]

Yep

Status:Needs review» Reviewed & tested by the community

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.