Add all views hooks to hook_hook_info()

+++ b/core/modules/views/lib/Drupal/views/Tests/ViewsHooksTest.phpundefined
@@ -16,42 +16,87 @@
+   * The moduel handler to use for invoking hooks.

module is spelled wrong

+++ b/core/modules/views/lib/Drupal/views/Tests/ViewsHooksTest.phpundefined
@@ -16,42 +16,87 @@
+  protected $testView;
...
+    $this->testView = views_get_view('test_view');

Is this actually needed?

+++ b/core/modules/views/lib/Drupal/views/Tests/ViewsHooksTest.phpundefined
@@ -16,42 +16,87 @@
+  public function setUp() {

If you're rerolling, make this protected :)

+++ b/core/modules/views/lib/Drupal/views/Tests/ViewsHooksTest.phpundefined
@@ -16,42 +16,87 @@
+        break;
+        case 'alter':
+          $data = array();
+          $this->moduleHandler->invoke('views_test_data', $hook, array($data));
+        break;
+        default:

break should be indented two spaces, and have a blank line after

+++ b/core/modules/views/views.moduleundefined
@@ -821,26 +821,24 @@ function &views_get_current_view() {
+  $hooks = array (
+    'views_data',
+    'views_data_alter',
+    'views_query_substitutions',
+    'views_form_substitutions',
+    'views_analyze',
+    'views_pre_view',
+    'views_pre_build',
+    'views_post_build',
+    'views_pre_execute',
+    'views_post_execute',
+    'views_pre_render',
+    'views_post_render',
+    'views_query_alter',
+    'views_invalidate_cache',
   );
 
-  return $hooks;
+  return array_fill_keys($hooks, array('group' => 'views'));

Nice!

Aweseome patch!

+++ b/core/modules/views/lib/Drupal/views/Tests/ViewsHooksTest.phpundefined
@@ -16,42 +16,83 @@
+          $this->moduleHandler->invoke('views_test_data', $hook);

The problem with invoke in general though is that it uses function_exists() first, so you actually can't be sure that the file in views.inc haven't been loaded, as once it's loaded it's loaded, but yeah no idea how to address the problem here.

+++ b/core/modules/views/lib/Drupal/views/Tests/ViewsHooksTest.phpundefined
@@ -16,42 +16,83 @@
+      $this->assertTrue(state()->get('views_hook_test_' . $hook), format_string('The %hook hook was invoked.', array('%hook' => $hook)));

state() that from the container ;)

$this->container->get('state')

This is ready to go now (with green).

#6: 1914256-6.patch queued for re-testing.

Then let's also adapt the other instances of it, to be more constistent.

This is RTBC now.

This puts the cached/info hooks and the runtime hooks into the same group, which means the info/cached hooks will end up more likely to be loaded on every request rather than less (although sites without views enabled at all could skip all of them).

We don't have any standards for what goes in hook_hook_info() but it was originally for performance (sometimes I put .admin.inc and .install files in apc.filters to keep APC size down given those are extremely low hit-rate files, potentially 'info' hook groups could be added to that). Since it doesn't account for dynamic hooks at all that really seems the only decent use for it apart from some minimal code organization.

What about using .views_runtime.inc additional to the already existing .views.inc, which should be the static one, which is not needed often.

Yeah that one is totally a runtime hook.

It didn't apply due to #1912946: Clean up comments in comment.views.inc node.views.inc user.views.inc, just a quick reroll.

#26: 1914256-26.patch queued for re-testing.

#26: 1914256-26.patch queued for re-testing.

+++ b/core/modules/views/views.moduleundefined
@@ -819,24 +819,27 @@ function &views_get_current_view() {
+  ), array('group' => 'views'));
...
+  ), array('group' => 'views_runtime'));

I would flip these... most other modules have their run-time equivalents in just "module.views.inc", and their admin stuff in "module.admin.inc". (Maybe "module.views_admin.inc" in this case?) "views_runtime.inc" is very strange, and passes a bunch of implementation details down the stack to "normal" developers.

+++ b/core/modules/views/views.moduleundefined
@@ -819,24 +819,27 @@ function &views_get_current_view() {
+    'views_analyze',
...
+    'views_invalidate_cache',

While the rest of these hooks make sense to run on every view that's shown on the front-end of the site, these two seem like oddball ones out. Shouldn't invalidate_cache be included with data/data_alter?

Also, does analyze belong here? It seems like it's only called from views_ui.module.

hook_views_analyze() and hook_views_invalidate_cache() are only invoked during Views administrative operations, so they are not needed as frequently as the Views runtime hooks. They'd be better grouped with the other hooks.

No strong opinion on the labels for the groups.

Edit: Sorry, crossposted. I tend to accumulate a ton of half-written comments on Friday mornings. ;)

Filed #1962704: Document hook_views_analyze().

Trying to think of a better name for views_runtime. These are the Views execution hooks (#1810590: Improve naming and documentation to clarify hook execution order for exciting details). Maybe we could call the group view_render or view_execution or something?

I really like the view_execution name, because it refers to the ViewExecutable.

+1 to views_execution (plural).

Lol. xpost.

+1.00000000000001

Ok, this looks good.

Alex explained that although hook_views_data() and their ilk are called less frequently, they're definitely defined much more frequently than any of the executable hooks, so it makes sense for them to take up the primary "views" namespace.

I also struggled a bit with "executable" as the adjective here, since that can also mean "something that can be killed" ;) whereas "runtime" is more explicit what's meant. But xjm pointed out that the underlying API name is "ViewsExecutable," so keeping these in sync is useful. Makes sense.

Committed and pushed to 8.x. Thanks!

Moving to the Views queue for the change notice there.

This seems really odd. As a module developer, I have a hook that does not have 'views_executable' in the name, but now I have to figure out that I cannot just place it in my modulename.views.inc file. This breaks the current pattern of core by just putting everything into the one namespace for consistency and DX. Both hook_token_info() and hook_tokens() are placed in modulename.tokens.inc even though one is the registry of information, and one is the 'executable'. I would hate it as a module developer if I had to remember each time as a best practice to have both pants.token_info.inc and pants.token_executable.inc.

tl;dr: It's a lot easier for a module developer to remember 'This is a Views hook, it should go in modulename.views.inc' rather than 'This should either go in modulename.views_execution.inc or modulename.views.inc.'

I feel very -1 on this change even though it's been committed.

I don't disagree on the DX impact, but this split was specifically asked for by catch in #16, for performance.

:/

Don't nail be down on that, but afaik you couldn't place the hook_views_FOO_BAR hooks into .views.inc, so at least that's not worse then in D7.

I agree that it's bad for DX, but seriously, we included probably more then 2000 lines of code or even more, without ever have the need for it.

Actually, what we should do is document the appropriate file on each hook in views.api.php.

Updated issue summary.

Moving to the core issue queue.

Filed the requested follow up from #45, #3375847: Document in views.api.php for each hook in which include file they can be placed .

Discussed with @xjm and @longwave, issuing a CR now would serve no purpose, so closing this as fixed.

Restoring original title and removing the now-irrelevant tag. Thanks!