- Issue created by @Lendude
Nikolas Haliotis β made their first commit to this issueβs fork.
- last update
about 1 year ago Custom Commands Failed - @nikolas-haliotis opened merge request.
- Status changed to Needs review
about 1 year ago 2:51pm 20 July 2023 - Status changed to Needs work
about 1 year ago 4:17pm 20 July 2023 - last update
about 1 year ago 29,833 pass - Status changed to Needs review
about 1 year ago 9:12am 21 July 2023 - Status changed to RTBC
about 1 year ago 3:22pm 21 July 2023 - πΊπΈUnited States smustgrave
Change matches IS and passes tests now.
- last update
about 1 year ago 29,878 pass - Status changed to Needs work
about 1 year ago 6:27am 24 July 2023 - last update
about 1 year ago 29,877 pass - Status changed to Needs review
about 1 year ago 7:12am 24 July 2023 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 fromsrc/Plugin/views/query/Sql.php
-
form_substitutions
Invoked fromsrc/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
about 1 year ago 12:05pm 24 July 2023 - πΊπΈ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 hooksAlso 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
inviews.api.php
: Aviews_overview
group and aviews_plugins
group. I was suggesting maybe creating two new groups, aviews_execution_hooks
and aviews_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!