Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
1) hook_update_N() implementations should have an 'Implements HOOKNAME()' first line the same as any other hook, both for DX and for the API module to find them easily.
2) Implementation and technical detail can not be added to the docblock because it is displayed in entirety to the user.
Both of the above are changes to the existing coding standards for update_hook_N().
Steps to reproduce
Proposed resolution
1) Add Implements HOOKNAME() to update_hook_N() implementation and ignore that line in update_get_update_list().
2) Only subset of the docblock should be used for the UI. Needs work to decided how to identify the lines to display.
Remaining tasks
Patch
Review
Commit
User interface changes
API changes
Data model changes
Release notes snippet
| Comment | File | Size | Author |
|---|---|---|---|
| #9 | 1663218-9.patch | 1 KB | quietone |
Comments
Comment #9
quietone CreditAttribution: quietone as a volunteer commentedThe IS lists wo problems
I think this is the line that 'doesn't connect'.
The documentation block preceding this function is stripped of newlines and used as the description for the update on the pending updates task list, which users will see when they run the update.php script.That can be improved by stripping of new lines phrase is needed.
The documentation block preceding this function is used as the description for the update on the pending updates task list, which users will see when they run the update.php script.This is fixed by #1624278: cleanup of docblock to UI text in update_get_update_list() is weak. Multi line doc block should display correctly now.
Not running tests until there is agreement on the text.
Comment #10
LendudeThe title and IS could use some more updating to make it clearer what he scope here is now.
New suggested text looks good to me in light of #1624278: cleanup of docblock to UI text in update_get_update_list() is weak
Comment #11
joachim CreditAttribution: joachim as a volunteer commented> the function's docs on api.d.o doesn't connect with
It took me a moment to remember what I meant by this, sorry!
What I mean is that the api.d.org page for the hook_update_N() doesn't list implementations of the hook, because the docblock of implementations doesn't say 'Implements hook_update_N()' as is standard for other hooks.
And the docblocks can't say that because then that 'Implements hook_update_N()' would end up in the user-facing text, which we don't want.
Comment #12
quietone CreditAttribution: quietone as a volunteer commentedfor 1) The api pages for hook_update list the functions that implement hook_update_N. For hook_update_N/9.2.x all the hook_update_N functions have a docblock that does not say 'Implements ...'. The same is true for D8.9 and D7. So, presumably everyone is well behaved and the review process and the committers prevent a docblock of 'implements ..' in a hook_update_N. There is a coding standard for this, Update functions (implementations of hook_update_N()) although I didn't check to see how long that has been approved. I don't know if there is a sniff for that either.
Finding this issue made me read module.api.php and see that it a minor improvement can be made.
Better?
Comment #13
quietone CreditAttribution: quietone as a volunteer commentedFix some typos
Comment #14
joachim CreditAttribution: joachim as a volunteer commented> The api pages for hook_update list the functions that implement hook_update_N. For hook_update_N/9.2.x all the hook_update_N functions have a docblock that does not say 'Implements ...'. The same is true for D8.9 and D7. So, presumably everyone is well behaved and the review process and the committers prevent a docblock of 'implements ..' in a hook_update_N. There is a coding standard for this
Ok, but my point is that the api page for hook_update_N() doesn't list implementations because we don't do this, and it should, and therefore, implementations of this SHOULD have the 'Implements ...' first line, which the UI should ignore.
Comment #15
quietone CreditAttribution: quietone as a volunteer commented@joachim, sorry, but I am not following. Can you provide a link to the page to be sure I am looking at the same one and then describe what you want and don't want to see.
Comment #16
joachim CreditAttribution: joachim as a volunteer commentedOk it looks like API module has had this improved since I filed this issue.
Back when I filed it, the API page at https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Extension... didn't show you the implementations of hook_update_N().
Now it does, but it looks pretty broken still -- action_post_update_move_plugins()? views_ui_ajax_update_form()? Those look totally wrong!
My point is that hook_update_N() implementations should have an 'Implements HOOKNAME()' first line the same as any other hook, both for DX and for the API module to find them easily. The code that slurps up the docblock for the UI should be changed so that it ignores this line.
The second point is that slurping up the whole of the docblock for the UI means that the docblock can't be used for implementation details which are important to developers but not admin users running an update. There should be a way to have the slurping code stop and ignore some of the text.
Comment #17
quietone CreditAttribution: quietone as a volunteer commentedAh, thank you! I was definitely stuck there. And I love the use of 'slurping', very accurate.
I have radically changed the IS. And I think this is a Feature Request because it is suggesting a change to a coding standard. Agree?
Note the api page does warn that functions other that hook_update_N() will be shown
Note: this list is generated by pattern matching, so it may include some functions that are not actually implementations of this hook.Comment #18
quietone CreditAttribution: quietone as a volunteer commentedTrying to follow the Coding standards process here. Moving to coding standards project for discussion.
Comment #19
joachim CreditAttribution: joachim as a volunteer commentedI am wondering whether at this point, we should postpone this until Drupal uses PHP8 attributes, as then we could do something like:
- normal docblock is just a docblock for developers
- UI text is in a function annotation