No alter hooks are invoked for views handler plugin definitions

AWESOME a unit test!

+++ b/core/modules/views/tests/src/Unit/ViewsHandlerManagerTest.php
@@ -0,0 +1,53 @@
+ * Tests the ViewsHandlerManager class.
+ *
+ * @group views
+ */
+class ViewsHandlerManagerTest extends UnitTestCase {
+

Let's add a @coversDefaultClass

Awesome!

Where's the hook documented in views.api.php?

1. +++ b/core/modules/views/src/Plugin/ViewsHandlerManager.php
   @@ -59,6 +59,7 @@ public function __construct($handler_type, \Traversable $namespaces, ViewsData $
   +    $this->alterInfo('views_handlers_' . $handler_type);
   

   Would views_HANDLER_TYPE_handlers_alter() be a better hook name than views_handlers_HANDLER_TYPE_alter()? Because what we're altering is handlers.

2. +++ b/core/modules/views/views.api.php
   @@ -1081,5 +1081,101 @@ function hook_views_plugins_wizard_alter(array &$plugins) {
   + * @param array $handlers
   + *   An array of all the existing handler definitions, passed by reference.
   

   What's not clear to me is whether the handler list for each hook is limited to the handlers for that type, or what its array structure is or where I can find it in the API.

3. +++ b/core/modules/views/views.api.php
   @@ -1081,5 +1081,101 @@ function hook_views_plugins_wizard_alter(array &$plugins) {
   +function hook_views_handlers_area_alter(array &$handlers) {
   ...
   +function hook_views_handlers_argument_alter(array &$handlers) {
   ...
   +function hook_views_handlers_field_alter(array &$handlers) {
   ...
   +function hook_views_handlers_filter_alter(array &$handlers) {
   ...
   +function hook_views_handlers_relationship_alter(array &$handlers) {
   ...
   +function hook_views_handlers_sort_alter(array &$handlers) {
   

   The list of all handler types from Views::getHandlerTypes(): field, argument, sort, filter, relationship, header, footer, empty. (Header, footer, and empty are all area handlers.) So all of them are covered. Cool.

   What if we add an additional handler type in the future? The handler type would automatically be altered, but the specific alter hook would be undocumented.

   Would it make sense to document it only once, along the lines of hook_field_widget_WIDGET_TYPE_form_alter()? We'd need a more detailed code example and explanation (see below).

4. +++ b/core/modules/views/views.api.php
   @@ -1081,5 +1081,101 @@ function hook_views_plugins_wizard_alter(array &$plugins) {
   +  // Change the 'title' handler class.
   +  $handlers['title']['class'] = 'Drupal\\example\\ExampleClass';
   

   It's a bit confusing to have this example for all six of the hooks. To the reader, the "title handler class" doesn't seem to have anything to do with altering the list of sort handlers or whichever. Can we come up with a better example, and explain how which handler type is being altered plays a role?

Re: #9 point 3, OTOH, the list of field widget types is a lot more in flux than views handler types, since AFAIK contrib cannot add a handler type (can it?), just handlers of one of the existing types. (Views::getHandlerTypes() is an unalterable static for a protected class property that is not touched anywhere else in the class; you'd have to override Views or ViewExecutable to change it as far as I can tell.)

So maybe the specific hooks are good.

I should also note that I'm not sure about #9 point 1; just raising the question.

Would views_HANDLER_TYPE_handlers_alter() be a better hook name than views_handlers_HANDLER_TYPE_alter()? Because what we're altering is handlers.

Well, yeah both would work. Plugins also do hook_views_plugins_$plugintype_alter, so I think this is fine, unless you want to change both instances.

What's not clear to me is whether the handler list for each hook is limited to the handlers for that type, or what its array structure is or where I can find it in the API.

Well it is limited, not sure whether this is worth to mention. I think this is what you expect it to be.

Would it make sense to document it only once, along the lines of hook_field_widget_WIDGET_TYPE_form_alter()? We'd need a more detailed code example and explanation (see below).

It's a bit confusing to have this example for all six of the hooks. To the reader, the "title handler class" doesn't seem to have anything to do with altering the list of sort handlers or whichever. Can we come up with a better example, and explain how which handler type is being altered plays a role?

I don't see why, this is pretty much all you can do here, replace the class. There isn't more metadata worth to change. It also doesn't make sense to have some if() as you can just change them globally anyway.

Well it is limited, not sure whether this is worth to mention. I think this is what you expect it to be.

Well clearly it's not, because I had no idea reading the list. :) And where can I learn more about the structure of the list? Is there a method that returns it?

I don't see why, this is pretty much all you can do here, replace the class. There isn't more metadata worth to change. It also doesn't make sense to have some if() as you can just change them globally anyway.

The problem is that reading the docs, I actually have no idea how I'd use the hooks. Maybe you could describe an example of how I'd use it, and I can help incoprorate it into Damian's docs?

I actually have no idea how I'd use the hooks.

I don't see why, this is pretty much all you can do here, replace the class.

Just to ensure we know what we talk about here: The only usecase is to replace an existing handler class, I can't think of any other one.
In the new handler class you do some arbitrary other logic, for example change the way how the title is rendered (in case of field handlers)

This needs to adapt views_hook_info() which currently defines these as hook_views_plugins_HANDLER_TYPE_alter. OTOH I wouldn't mind having hook_views_plugins_*_alter for views handler plugins.

I think we should remove dynamic hooks from views_hook_info() for now. Dynamic hooks really shouldn't be in there - it's supposed to be for lazy loading of hook code (which is questionable in 8.x anyway) as well as just organisation, and doing work to identify the code to be lazy loaded quickly cancels any benefits out.

I agree with olli, let's use hook_views_plugins_$type_alter() as we already define it.

I think we should remove dynamic hooks from views_hook_info() for now. Dynamic hooks really shouldn't be in there - it's supposed to be for lazy loading of hook code (which is questionable in 8.x anyway) as well as just organisation, and doing work to identify the code to be lazy loaded quickly cancels any benefits out.

I disagree because it makes your code a bit less easy to read. hook_views_data and hook_views_plugins_$type_alter()work together, so having them in
one place helps quite a bit in terms of readabilty. At least for the later hook there is though indeed no point in lazyloading, the amount of extra code is basically zero.

damian++

Committed 695afed and pushed to 8.0.x. Thanks!

diff --git a/core/modules/views/tests/src/Unit/ViewsHandlerManagerTest.php b/core/modules/views/tests/src/Unit/ViewsHandlerManagerTest.php
index 5b094a4..eba45da 100644
--- a/core/modules/views/tests/src/Unit/ViewsHandlerManagerTest.php
+++ b/core/modules/views/tests/src/Unit/ViewsHandlerManagerTest.php
@@ -47,7 +47,7 @@ public function setUp() {
    * @covers ::__construct
    * @covers ::getDefinitions
    */
-  public function testAlterHookInvokation() {
+  public function testAlterHookInvocation() {
     $this->moduleHandler->expects($this->once())
       ->method('alter')
       ->with('views_plugins_test', array());

Fixed minor spelling mistake on commit.

I was fine with using german here :)