Document in views.api.php for each hook in which include file they can be placed

Created on 20 July 2023, over 1 year ago
Updated 27 July 2023, over 1 year ago

Problem/Motivation

Follow up to πŸ“Œ Add all views hooks to hook_hook_info() Fixed .

All hooks views.api.php in should document in which .inc file they should be placed, see views_hook_info() for the correct locations, either MODULE_NAME.views.inc or MODULE_NAME.views_execution.inc

Steps to reproduce

Proposed resolution

Update all hook documentations in views.api.php to include the correct include file to use. For example do something like:

/**
 * Analyze a view to provide warnings about its configuration.
 *
 * Implementations of this hook should be placed in MODULE_NAME.views.inc.
 *
 * @param \Drupal\views\ViewExecutable $view
 *   The view being executed.
 *
 * @return array
 *   Array of warning messages built by Analyzer::formatMessage to be displayed
 *   to the user following analysis of the view.
 */

Remaining tasks

In this issue

  • Separate the hooks into two documentation groups: one for the runtime hooks (those invoked from ViewExecutable, which go in .views_execution.inc) and the others (which go in .views.inc). Add introductory documentation to each group explaining the differences between the two groups and the purpose of separating them (performance). Reference πŸ“Œ Add all views hooks to hook_hook_info() Fixed

Followups needed

  • Document the field hooks separately
  • Deprecate the include file pattern in favor of a different method of on-demand autoloading for the respective hooks

User interface changes

API changes

Data model changes

Release notes snippet

πŸ“Œ Task
Status

Needs work

Version

11.0 πŸ”₯

Component
ViewsΒ  β†’

Last updated about 1 hour ago

Created by

πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

Live updates comments and jobs are added and updated live.
  • Novice

    It would make a good project for someone who is new to the Drupal contribution process. It's preferred over Newbie.

Sign in to follow issues

Comments & Activities

  • Issue created by @Lendude
  • Nikolas Haliotis β†’ made their first commit to this issue’s fork.

  • last update over 1 year ago
    Custom Commands Failed
  • @nikolas-haliotis opened merge request.
  • Status changed to Needs review over 1 year ago
  • Status changed to Needs work over 1 year ago
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    CC failure

  • last update over 1 year ago
    29,833 pass
  • Status changed to Needs review over 1 year ago
  • Status changed to RTBC over 1 year ago
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    Change matches IS and passes tests now.

  • last update over 1 year ago
    29,878 pass
  • Status changed to Needs work over 1 year ago
  • πŸ‡³πŸ‡ΏNew Zealand quietone
  • last update over 1 year ago
    29,877 pass
  • Status changed to Needs review over 1 year ago
  • Removed implementation details for the field hooks and added the implementation details to the missing hooks.

  • πŸ‡ΊπŸ‡ΈUnited States xjm

    Thanks @quietone, I had also gotten hung up on the field hooks when reviewing this. Here's what I had documented:

    • query_substitutions
      Invoked from src/Plugin/views/query/Sql.php
    • form_substitutions
      Invoked from src/Form/ViewsFormMainForm.php
    • Invoked directly from ViewExecutable:
      • views_pre_render
      • views_post_render
      • views_pre_build
      • views_post_build
      • views_pre_view
      • views_pre_execute
      • views_post_execute
    • Also in ViewExecutable:
          // Let modules modify the query just prior to finalizing it.                
          $this->query->alter($this);
      

      ...which gives us views_query_alter.

  • πŸ‡ΊπŸ‡ΈUnited States xjm

    @Lendude and I also discussed a followup for this. The use of .inc files is archaic, and even in the original issue it was identified that these two magically named include files were bad DX. Their purpose is to avoid loading code that is not needed on a given request, but nowadays we should be able to come up with a better pattern for that with a modern architecture and better DX. To be discussed in the followup. :)

  • Status changed to Needs work over 1 year ago
  • πŸ‡ΊπŸ‡ΈUnited States xjm

    A thought: Should the runtime hooks and the non-runtime hooks furthermore be placed in different phpdoc groups? Documenting it on the individual hooks is good for when someone just looks up the individual hook, but if we add two defgroups, that will also give us a spot to document the "why" of the two different hook categories from the original issue.

    We should also perhaps have a second followup to document the field hooks differently (with their own docs and in their own third group).

    Updating the remaining tasks with the above.

  • So my understanding is that we want the views.api.php file to be organised like so:

    -> a doc block at the start with documentation on the views
    -> all the hooks that are relevant
    -> a doc block with the documentation for the runtime hooks
    -> all the hooks that are relevant
    -> a placeholder doc block for the field hooks with a TODO
    -> all the field hooks

    Also there should be a task about the documentation of the field hooks after this, right?

  • πŸ‡ΊπŸ‡ΈUnited States xjm

    Thanks @Nikolas Haliotis for looking into this. :) Crossposting my remarks from #contribute in Drupal Slack:

    I was referring to the phpdoc @defgroup tags: https://www.drupal.org/docs/develop/standards/php/api-documentation-and-... β†’

    Currently, there are two @defgroup in views.api.php: A views_overview group and a views_plugins group. I was suggesting maybe creating two new groups, a views_execution_hooks and a views_something_else_hooks (needs a proper name). Each of those groups could have high-level explanations of the differences between the two kinds of hooks and why they get placed where they do.

    I do think the added paragraph on the individual hooks is also still useful, so we can leave it there. However, we would remove the changes to the two field hook definitions from the patch, and then have a followup to see if they should be documented in some other way based on how other field hooks are documented elsewhere (possibly an @ingroup or something else, but we don't need to figure it out in this issue, just create a separate issue before it's RTBC).

    Hopefully that helps!

Production build 0.71.5 2024