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.
Part of meta-issue #1310084: [meta] API documentation cleanup sprint
Comment | File | Size | Author |
---|---|---|---|
#123 | node_docs-1313980-123.patch | 74.27 KB | Albert Volkman |
#123 | interdiff.txt | 4.46 KB | Albert Volkman |
#119 | node_docs-1313980-119.patch | 512 bytes | Albert Volkman |
#113 | node_docs-1313980-113a.patch | 56.36 KB | Albert Volkman |
#113 | interdiffa.txt | 890 bytes | Albert Volkman |
Comments
Comment #1
jn2 CreditAttribution: jn2 commentedHere's the patch.
Comment #2
xjmHey @jn2, I'm working on the sprint as well, so I thought I'd help review your patch. Here are some thing I noticed:
I think based on http://drupal.org/node/1354#forms that these should have the actual form builder named in the summary, e.g. "Form submission handler for foo_form()."
I think these need to be changed to the form builder standards in http://drupal.org/node/1354#forms? You've already taken care of most of them, but looks like these were missed.
We should probably reword this so that the important points fit on one line, and the additional details are deferred to another paragraph.
This is just an observation from my own patch, but I realized in all cases the phrases "Helper function" and "Utility function" actually didn't add any meaning. I replaced them with normal summaries in the form "Verbs foo bar," and added @see references to other functions where appropriate.
Needs a verb, I think.
I don't think a verb is actually required for the constants' summaries. On the other hand, your changes to this and its friends probably make them clearer.
We should probably make this one line (the first sentence) and put the additional info in a separate paragraph.
For these I think "Form submission handler for the preview button of node_form()" might be better, as I don't see this pattern documented.
Nice job on this. The node module is a big one. :)
Comment #3
jn2 CreditAttribution: jn2 commentedThanks for the review, xjm. Here's a new patch that should address all your comments.
Comment #4
xjmThanks @jn2. I read through your updated patch and found a few more small things:
Here's another couple form submission handlers.
Maybe "Updates individual nodes with the supplied values" or something along those lines?
Looks like these summaries are not one line.
It looks like in all the node actions except node_unpublish_by_keyword_action() and node_assign_owner_action(), the passed $context is not actually used. I wonder if we should make that clear somehow here?
In the action group documentation, the documentation for $context is "Array of additional information about what triggered the action." Maybe we could use that text, followed by "Not used for this action" for the actions where it isn't used, and followed by an explanation of the expected keys in those other two? What do you think?
I think these need to be switched over to the standard for form builders.
Okay, I could be wrong about this because it confuses me, but based on #1315886-31: Clean up API docs for includes directory, files starting with A-C I think this should be "Overrides" rather than "Implements" since NodeController is extending a base class rather than implementing an interface.
Based on http://drupal.org/node/1354#themeable it sounds like the @return isn't necessary for themeable functions that return HTML, but it is a minor point. It also doesn't say that you shouldn't have it, so I am not sure which is preferred. :)
Maybe these can say "for the 'Foo' button of node_form()" rather than "on the node form" like the others you changed already.
Comment #5
jn2 CreditAttribution: jn2 commented@xjm
Thanks again for the second pair of eyes. Hehe. Fixed part of the issue on those last two, but not the whole thing.
Big help pointing me to the themeable section of 1354. I went back and redid almost all the theme functions I had edited.
I really didn't know what to do with those $context parameters. Didn't notice the @ingroup actions. (Doh!) So that was a very good set of changes.
I think you are right about 'overrides' as well.
Took on the node module because I thought I'd learn something, and that has been very true!
Comment #6
jn2 CreditAttribution: jn2 commentedComment #7
xjmOops, I think this is supposed to be @ingroup.
Other than that, the patch is looking pretty good to me! Leaving at NR for review from the docs folks.
Comment #8
jhodgdonLooks pretty good, thanks! And I'm not sure who these "docs folks" are -- aren't we all? :)
A couple of things to fix:
a)
Needs verbification of first line.
b)
This is both a page callback and a form generating function (actually, the function doesn't delete anything, it just generates the form). So the first line isn't quite right -- I suggest combining with the Form Generation standards on http://drupal.org/node/1354#forms to make something like this for the first line:
Page callback: Form constructor for the content type delete form.
Same for node_configure_rebuild_confirm() lower down in the patch.
c) Punctuation nitpick:
"View mode, e.g. 'full', 'teaser'..." --> e.g. needs a comma after it (punctuate as if it was "for example"). This applies to several spots in the patch.
d) Might need "the" in here, maybe twice:
Checks if user has proper permissions to access node add form.
Maybe here too:
Title callback: Displays node's title.
And here:
Assesses if current user may perform a certain operation on a specific node.
(actually "if" should be "whether"?)
I may not have read the rest of the patch as carefully... you might take a quick check and see if anything else needs "the"?
e) This line needs to be wrapped (put the link on its own line so it doesn't have a line break in it):
Comment #9
jn2 CreditAttribution: jn2 commented@jhodgdon
The reason many of those lines don't include 'the' is the 80 character limit. Same for 'if' in place of 'whether'. I'm afraid this is the best I can do to fit those sentences into 80 characters. If you can write it with 'the' and fit it into 80 chars, I'll be happy to change the patch. Or if 'the' is more important than line length, I can do that.
Comment #10
jhodgdonI'm not in favor of bad writing style, or of lines longer than 80 characters in the first-line description. In the past, we have usually been able to fix both at the same time by being creative. :)
Comment #11
jhodgdonOK, here are suggestions for the examples I cited above:
Checks if user has proper permissions to access node add form.
-->
Checks whether the user has permission to add nodes.
Title callback: Displays node's title.
-->
Title callback: Displays the node's title.
Assesses if current user may perform a certain operation on a specific node.
-->
Checks whether the user has permission to perform an operation on a node.
Comment #12
jn2 CreditAttribution: jn2 commented@jhodgdon
Thanks for the suggestions. I should have looked a bit closer; it was really only the third one that was driving me crazy.
So here's the revised patch with xjm's last suggestion and jhodgdon's suggestions. I read through and added articles and otherwise cleaned up the language as well.
Comment #13
xjmAlright, based on @jhodgdon's feedback above I did a close read on #12 looking for missing articles and such. Some of the following is somewhat tangential to the sprint, so let me know if you think we should change it in this patch or not.
Fewer than 10 what? :) How about: "Updates individual nodes when fewer than 10 are queued."
Based on what @jhodgdon says above, I think we could add articles here as well.
Edit: Typo preserved for posterity because it is ironic:
I think these should fit 80 chars:
While we're changing these lines, I think it should be node_load() and node_load_multiple() (with parens)?
How about, "The ID of the node if this is a full page view, or FALSE otherwise."
"TRUE if the user has permission to access the form, or FALSE otherwise."
If these are the possible string values of keys, maybe it would be clearer to add single quotes around them: "An associative array containing 'title', 'link', 'description', and other keys..." What do you think?
If we're adding articles, this could be "The node object on which the operation is to be performed, or the node type for the 'create' operation." (Again with the single quotes too.)
We could add parens here for hook_update_N(), hook_user(), and hook_taxonomy().
Hmm, bit weird with the one floating quotation mark and the variable here. Maybe just "node/add/%"?
I think this is another one of those hybrid menu/form constructor dealies.
Thanks so much for all your patience with the rerolls. :)
Comment #14
xjmNote that this patch will need to be rerolled, because the core directory structure for Drupal 8 has now changed. (For more information, see #22336: Move all core Drupal files under a /core folder to improve usability and upgrades). If you need help rerolling this patch, you can come to core office hours or ask in #drupal-gitsupport on IRC.
Comment #15
jn2 CreditAttribution: jn2 commentedHere's the patch re-rolled for the new directory configuration and with changes from #13.
Most of the changes in #13 were good, but I didn't use them all. Some differences were just a couple of words, but two might deserve explanation:
I think this is personal preference. The phrase 'access the form' is just one line up. I think it's clearer without the extra words.
This needed to be changed, but not as suggested. My impression was the reason for including paths was to locate the callback in the hook_menu() implementation. The suggested change isn't the way it's called in node_menu. But I did need to change it, include the first single quote and explain that it's part of a foreach. With that it's easy to find in node_menu.
Comment #16
jhodgdonI took some time to review this whole patch carefully. It looks pretty good. A few things I noticed:
a) The first hunk in the patch is for node_overview_types(). This is actually a page callback function for admin/structure/types, so it should be documented according to http://drupal.org/node/1354#menu-callback
b)
The one-line description and the @return don't seem to match here? One says you get choices, other says you get "the length of the teaser", which sounds like it's one length. Needs clarification.
c)
This looks a bit redundant - applies filters for filters?
d)
I generally don't like to see variable names in one-line function descriptions. These are displayed all over api.drupal.org in listing pages, and out of context, the variable names don't make as much sense.
e) node_admin_nodes_validate() and _submit() should have @see pointing to each other, and node_admin_nodes() should have @ingroup forms on it.
http://drupal.org/node/1354#forms
f)
In modern American standard usage, the - line here is preferred to the rather convoluted wording of the + line, at least according to my usage references.
g)
Should these be "Denotes that the node is..."? Same applies to all of the "Denotes" comments below this.
Similarly:
+ * Prepares node for saving by populating author and creation date.
==> Prepares a node
h)
This doesn't really need the @see. The reference is made in the text above, with context to explain why.
i)
Pet peeve of mine: "uid" is not really acceptable in text (nor "id", "nid", etc.). They should all be "ID", and if the context is not clear, something like "node ID", etc.
j)
This is missing some subjects and verbs.... actually it seems wrong?
That's about where I stopped. In general, can you read through the new text you are adding and make sure that it stands alone as good English text rather than being so terse as to omit words?
Comment #17
jn2 CreditAttribution: jn2 commented@jhodgdon
I'd like to understand the function in b) better. In trying to determine what needed to be in the docblock, I looked to see where this function is called. In the API, it's not listed as called anywhere. Is this unused legacy code? Or is there something here I'm not understanding?
Comment #18
xjmSo it's legacy and no longer used. I'd just not touch the docblock for it, and open a followup issue against node.module to remove the function.
Edit: For the curious, this function was used to format the D6 and earlier sitewide teaser length options list: "Unlimited," "100 characters," etc. In D7 this functionality has been moved to the Field API and the user just enters a number instead.
Comment #19
jhodgdonGood detective work xjm - not touching the docblock and filing a follow-up issue sounds like the best course of action. :)
Comment #20
jn2 CreditAttribution: jn2 commentedHere's the patch with revisions from #16. Also went through everything again looking for overly terse language.
Comment #21
jhodgdonThis is looking really good now, thanks!
I found one small thing:
Paragraph wrapping -- 4th line should be wrapped with 3rd etc.
If you're going to fix that, you could also fix this:
Normally, we don't use '' on lists of array elements. See http://drupal.org/node/1354#lists -- I wouldn't suggest updating it (out of scope for this clean up effort), except this is a new list that this patch adds that didn't exist before...
And while we're at it, do you think that node_form() should have @see for node_form_delete_submit() and the other button submission handlers, as well as node_form_submit()? I think it should...
Comment #22
xjmYeah, I'd agree that this is a good idea.
Comment #23
jn2 CreditAttribution: jn2 commentedHere's a patch with the changes from #21.
- 'owner_uid': User ID assigned to the node, if any.
was cut/pasted from a nearby function. There were actually three with the quotes, so I fixed them all for consistency.Thanks for all the helpful comments. For me at least, it's hard to keep track of everything over so many files and functions. Definitely helps to have extra sets of eyes on the patch.
Comment #24
jn2 CreditAttribution: jn2 commentedComment #25
xjmI think we might still be missing some inter-form handler
@see
references. There sure are a lot in the node module! I'll try to to come back to this later tonight and help look for specifics.Comment #26
xjmAlright, here are all the form "families" I found:
(some of these are hook implementations or alters and not actually related to one single form)
The actions aren't themselves normal form builders but should likely have @see relationships with their validators if they don't already.
node_admin_nodes()
actually just returns the form object tonode_admin_content()
, which is the actual constructor.node_filter_form()
is similarly called bynode_admin_content()
.So within each family, the main form should presumably reference its validators and submission handlers for all buttons, as well as its confirmation forms, etc? And each confirmation form then references the parent form as well as its own validation and submission handlers.
I also noticed a few oddities. One is
node_search_validate()
, which claims it's a "Form API callback for the search form. Registered in node_form_alter()." That generic form alter is gone, andnode_search_validate()
is now a validation handler fornode_form_search_form_alter()
. (It might be worth looking at all the form alters in the module to double-check the submission and validation handlers they register.)Another thing is that the form alters are documented as
Implements hook_form_FORMID_alter()
. I think it's actually supposed to behook_form_FORM_ID_alter()
(with an underscore)?Edit 2: Done finally after some distraction.
Comment #27
jhodgdonYes on the FORM_ID_alter, before I forget.
Thanks for the detective work xjm. Guess we should set this to needs work to make sure all those @see are there and the FORMID->FORM_ID thing.
Comment #28
jn2 CreditAttribution: jn2 commentedHere's the patch.
Edited: This isn't quite done yet.
Comment #29
xjmSee also: #1337282: Lacking doc header standard for forms used as page callbacks
Comment #30
jn2 CreditAttribution: jn2 commented@xjm #29
I was already doing that. I read that as part of the original specification, although maybe it wasn't.
Comment #31
xjm@jn2: I pointed it out because it proposes doing what you have, except omitting the "Page callback: " before "Form constructor for..." on account of the fact tht the form constructors aren't actually the page callbacks;
drupal_get_form()
is. (If they were page callbacks, they'd presumably return markup rather than a FAPI array.)Comment #32
jn2 CreditAttribution: jn2 commentedI see. I didn't read it closely enough. So I'll make those changes.
Comment #33
xjmWell, might want to wait until we come down on one side or the other on that question. :) Of course, it's also a simple thing to fix after the fact, too. So I'd wait and just finish whatever changes you were working on in #28.
Comment #34
jhodgdonSince this patch isn't done (as per author of patch), moving back to needs work.
Comment #35
jn2 CreditAttribution: jn2 commentedHere's a patch that should address all the issues from #26.
Comment #36
jn2 CreditAttribution: jn2 commentedComment #37
xjm63k, phew! This patch looks really solid, and given how much work has gone into it already, I think we should get this committed. :) If there's anything we want to refine further, we can open followup issues.
Awesome work on this. :)
Comment #38
catch#35: node_module_docs-1313980-35.patch queued for re-testing.
Comment #39
catchThat's a lot of clean-up!
Committed/pushed to 8.x.
Comment #41
xjmThis needs to be backported; however, it will not be an easy backport. :)
Comment #42
xjmComment #43
Albert Volkman CreditAttribution: Albert Volkman commentedI'll take it :)
Comment #44
Albert Volkman CreditAttribution: Albert Volkman commentedD7 backport.
Comment #45
xjmFound a few issues, unfortunately. Some are things that should not be backported, and others might be problems with the D8 patch that went in. (It was long so I know I might have missed things reviewing it.)
Variable data types for @param and @return should not be added to D7.
I think hook_ranking() needs parens. Is this in D8?
This change is grammatically incorrect and should be reverted. Was this in the D8 patch?
The summary change here is probably okay for backport, but not the @see.
I think this is just supposed to say (e.g.) "Implements hook_form_FORM_ID_alter() for block_custom_block_delete()" -- if this is what went into D8, I guess we could open a followup to change it.
I think this change is also probably not supposed to be backported (sinc it's a "new" standard for D8).
The @see module_menu() need to go away for the backport, I think.
Shouldn't this be
@ingroup forms
? Did it go into D8 like this as well?Comment #46
jhodgdonReally, we can't have @see node_menu() in there in D7? I think those can stay.
Comment #47
Albert Volkman CreditAttribution: Albert Volkman commentedI appreciate the review @xjm even with your low levels of sleep!
1) Removed.
2) Yep, D8 doesn't have the parentheses-
http://drupalcode.org/project/drupal.git/blob/refs/heads/8.x:/core/modul...
3) It was in the patch, but not committed. Fixed.
4) Removed @see. Just wondering, why should this be removed?
5) Same as D8-
http://drupalcode.org/project/drupal.git/blob/refs/heads/8.x:/core/modul...
6) Removed (as well as the other Page callbacks).`
7) Yeah, this is form in D8-
http://drupalcode.org/project/drupal.git/blob/refs/heads/8.x:/core/modul...
8) I've left this in place for a backport of the new issue.
Comment #48
jhodgdonRE the @see to node_menu(), I guess we have been removing them from other d7 ports, so that should be done. There are still quite a few in this patch.
Anyway, having the @see is less relevant than it used to be, since if you go to, for instance, node_overview_types on api.drupal.org, you get a link to the 1 "string reference", which takes you back to node_menu().
Comment #49
jhodgdonhad forgotten to change the status over the @see stuff.
Comment #50
Albert Volkman CreditAttribution: Albert Volkman commentedHere's my stab at fixing D8.
Comment #51
jhodgdonUm...
In the first line, it should name the form that is being altered. Which I think here is block_add_block_form()? Same for the others in this patch.
Comment #52
Albert Volkman CreditAttribution: Albert Volkman commentedWow, absolutely no excuse for that. My apologies.
Comment #53
jhodgdonbot trigger
Comment #54
jhodgdonWe are unfortunately over thresholds for critical/major issues right now, and I think this patch will conflict with the in-progress issue
#1268974: For disabled node types with 'base' != 'node_content', _node_types_build() does not return data about the type.
so I cannot commit it right now. Dang.
Comment #55
xjm#52 looks fine to me (though sounds like it might need to be rerolled). On the other hand, since it's only a few lines, if someone were to reroll that other issue's patch right after it were fixed... :)
Comment #56
jhodgdonActually, #1268974 is back to needs work anyway. I don't see any NR or RTBC patches in the critical/major queue that this will conflict with now, so I will go ahead and commit it.
So the patch in #52 is committed to Drupal 8.x. Putting back to 7.x, where I think the patch in #47 was pretty close.
Comment #57
Albert Volkman CreditAttribution: Albert Volkman commentedUpdated patch with an interdiff against #52.
Comment #58
Albert Volkman CreditAttribution: Albert Volkman commentedHrm... is it stuck?
Comment #59
jhodgdonLooks stuck. Try uploading the patch again.
Comment #60
Albert Volkman CreditAttribution: Albert Volkman commentedComment #61
xjmRelated followup: #1540094: Add missing docblocks for classes and methods in node.test
Comment #62
jhodgdonSorry for the delay... I finally got around to reviewing this. It is mostly fine, but I did find a couple of very minor things that should be fixed:
a) This change should not be made in node.module:
Adding "the" is OK, but it should say "reset" not "resets".
b) In several files, there are form generating functions that have arguments beyond $form and $form_state that are not documented, such as this one in node.pages.inc:
c) This is in node.pages.inc:
needs "the" in there.
d) In node.test, there is a class called NodeRevisionsTestCase that has no doc block, and this patch doesn't add one. It should, and its methods need verb tense updates. Actually, there are other classes after that with no documentation also in the same file.
e) In node.module:
$variables is an array. It doesn't have "arguments", it has elements, and they don't start with $. This should say:
An associative array with the following elements:
- node: (description)
- view_mode: (etc.)
Comment #63
Albert Volkman CreditAttribution: Albert Volkman commentedOkay, I've now addressed those issues in this patch. I unfortunately cannot create an interdiff as the original patch won't apply cleanly.
Comment #64
jhodgdonThanks! This still needs some work -- I don't think all of the items in #62 were quite addressed:
1. My mistake on (a) above -- I didn't notice:
forces -> force would be good. :)
2. Regarding (b)... Form constructors shouldn't bother with @return:
The @return should just be removed.
3. And this one isn't quite right: the parameter name is $node not $nodes:
4. node_delete_confirm() has the same problem as (3).
5. This one still needs an @param:
6. node_revision_delete_confirm() -- see (5).
7. Regarding (d), the tests are now in subdirectory core/modules/node/lib/Drupal/node/Tests. All of those files probably need to be checked for documentation (they used to all be part of node.test).
8. Regarding (e), we shouldn't be saying:
arguments -> elements
9. Looking at the existing node.module file, I think this same fix as (e) needs to be made in template_preprocess_node() also.
Comment #65
Albert Volkman CreditAttribution: Albert Volkman commented1-6 fixed.
For 7, there's no docblocks in D8, so should we revert back to D8 to add them there first? Or perhaps there's another issue already addressing this?
8 fixed.
9... isn't 8 referring to template_preprocess_node()?
Interdiff'd against #63.
Comment #66
Albert Volkman CreditAttribution: Albert Volkman commentedComment #67
jhodgdonRegarding (7), I must have been looking at the 8.x branch and not realizing this was a 7.x issue now, sorry! Probably yes we should go back to 8.x again and check to see that the tests (which are in subdirectories in 8.x) all have doc headers.
Comment #68
jhodgdonLet's go back to 8.x temporarily. I took a look at the test classes in core/modules/node/lib/Drupal/node/Tests, and quite a few of them are completely lacking documentation headers on the classes and their methods (or have non-compliant doc headers). So we should fix that before completing the backport.
Comment #69
jhodgdontag
Comment #70
Lars Toomre CreditAttribution: Lars Toomre commentedI would like to drive this issue home from a D8 perspective. Reading through the various comments, I am uncertain about remains to be done beyond what @jodgdon outlined in #68.
Can someone please elaborate and I will roll a patch plus doing a thorough review of what ever else I think needs to be addressed for this issue? Thanks.
Comment #71
Lars Toomre CreditAttribution: Lars Toomre commentedHere is a patch focused on the docblocks in the top level files in the Node module. This does not include any type hinting except where required.
My thought here is that incremental improvements will move this module towards fully complying with http://drupal.org/node/1354 documentation standards.
Once this patch is committed, I plan on rolling a corresponding patch in #1800546: Add missing type hinting to Node module docblocks that adds type hinting to each of the appropriate @param and @return directives.
Comment #72
Lars Toomre CreditAttribution: Lars Toomre commentedOh, I meant to say, in a couple of places, I used '???' wher I was uncertain. Suggested corrections are welcome.
Comment #73
jhodgdonThanks for the patch! This will take a little time to review in detail, since there is quite a bit of param/return documentation added... I'll try to get to it sometime this week if possible, if no one else beats me to it.
Meanwhile, I did notice this formatting problem (extra line) in a quick scan:
And regarding the ??? spots... I don't think I'd want to commit a patch with ??? in it... so let's see, here is some information (it needs to be made into documentation):
- The $operations params in a couple of batch-related functions -- see batch_set() for documentation of what that is.
- _node_query_node_access_alter() does not have a return value at all, ever, and should not have a @return at all.
- node_page_edit() is returning the node editing form array.
- node_add_page() is returning a render array for a list of the node types that can be added; however, if there is only one node type defined for the site, the function redirects to the node add page for that one node type and does not return at all.
Maybe you can fix the ??? with that information, and remove that blank extra line, and we can go from there? Thanks!
Comment #74
Lars Toomre CreditAttribution: Lars Toomre commentedThanks @jhodgdon. I expected that I would need to re-roll this patch. I will do so in morning when I am back at my development machine.
Comment #75
Lars Toomre CreditAttribution: Lars Toomre commentedAs promised, attached is a patch that addresses most of #73. It has removed that extra line and replaced '???' with appropriate text descriptions.
@jhodgdon, please take a close look at the code for _node_query_node_access_alter(). There are three early returns in that function. How would you like those cases documented?
Comment #76
Lars Toomre CreditAttribution: Lars Toomre commentedLet's remember to rebase before rolling the patch. Here is a smaller patch.
Comment #77
jhodgdonRE #175 - Yes, there are three early returns. But in no case does that function ever return a value -- there is never a "return " statement in the function. If a function does not return a value, it should not have a @return section in its documentation. See
http://drupal.org/node/1354#functions
So this just needs to be removed:
If you want to say something about whether the module is modifying the query in those cases, that is fine but it goes in the main function body, not in the return value documentation.
Comment #78
Lars Toomre CreditAttribution: Lars Toomre commentedAttached in a untested patch that addresses #77 along with an interdiff.
Following up on the comment #1787900-5: Clean up cruft in the Action documentation, I have added type hinting to those @param and @return directives that were added by this patch. I also noticed a couple of Default spacing problems which I also corrected.
Comment #79
Lars Toomre CreditAttribution: Lars Toomre commented@jhodgdon I am working today on the next patch for this big module focused on the multiple test files for the Node module.
I am confused about whether 'function test..." should be declared as "public function test..." By default, such functions are public anyways and I have seen some patches explicitly declaring them that way. Plus the examples at bottom of http://drupal.org/node/325974 use 'public function test...' as well.
However, most of the test methods in node module are silent on this issue. Care to give some direction on what should be done?
Comment #80
jhodgdonRE #79 - This initiative is only for documentation header changes outlined in the meta-issue only ==> absolutely no code changes. That is a question for the "Coding standards cleanup" initiative, which is separate, and also I don't know the answer to your question.
Comment #81
jhodgdonLooks good! I committed the patch in #78.
Can we have one more follow-up: in node.module that I noticed while reviewing this patch:
Should be node_access_rebuild_batch() -- not sure if this problem occurs elsewhere too?
Also, see #68 - I think we still have quite a bit of test documentation cleanup for this module, so leaving this issue open.
Comment #82
Lars Toomre CreditAttribution: Lars Toomre commentedI would like to get #1804522: Improve Tests consistency to standards in modules N-Z in before doing a further detailed review of the test files for this module. That issue catches many of the wrong verb tense issues in the test files as well as consistently documenting user login members.
I am adding an @todo item to my list to perform another review of the Node module now that #78 is in. I will re-roll a patch then that addresses #81 as well.
Comment #83
Lars Toomre CreditAttribution: Lars Toomre commentedI am working through the Node Test files today since there has been no movement on #1804522: Improve Tests consistency to standards in modules N-Z. I hope to have another incremental patch ready later today.
In the interim, I open and posted a patch to #1805624: Move assertNodeAccess() to WebTestBase.php Simpletest file. This small issue will effect the NodeAccessLanguageTest.php file.
Comment #84
Lars Toomre CreditAttribution: Lars Toomre commentedThis untested patch is the next in a series trying to clean-up the documentation in the Node module.
As stated in #83, this patch includes documentation fixes for all of the Node test files. Another reason this patch is so big is that includes corrections to list formatting and rewrapping numerous text lines to conform with 80 characters, particularly in node.api.php file.
I also did another read through of all of the other files in the Node module. I was quite surprised at the number of other things that I noticed needing further correction. Those changes have been included here as well.
Once this patch gets in, we probably will need one more careful read of the module's files and my guess is that then it should be good for backport to D7.
Comment #86
Lars Toomre CreditAttribution: Lars Toomre commentedFixes a stray character at top of SummaryLengthTest.php file. Let's see what further the bot thinks.
Comment #87
Lars Toomre CreditAttribution: Lars Toomre commentedI added a small patch in #1800546-2: Add missing type hinting to Node module docblocks that adds type hinting to the Node hook documentation in node.api.php file.
Comment #88
jhodgdonThanks for the patch!
Whew! That took a while to review... A couple of things to address:
a) I'll just make my usual comment here: this issue's patch should not be affecting any code lines [use statements reordering, adding/removing blank lines, getInfo() values, etc.] -- just documentation blocks. Moving @file blocks to the top of the file is OK, though. There are other issues to address those other problems on.
b)
Some of the @file that was there didn't get moved up to the top with the rest. This also happened in includes/database.inc and perhaps other files as well.
c)
I know that you are fond of alphabetizing @see statements (I'm not sure exactly why, since as far as I know we don't have a standard on this?)... but in this case the *logical* order is validate then submit, so I think it would be better just to leave this one as it was? Same with this change in another file:
d)
Class docs should also start with a verb, like functions. [NodeAccessTest, NodeSaveTest, NodeTestBase too]
e) NodeQueryAlterTest::testNodeQueryAlterOverride() -- there are two spaces after a . in the docs.
f) Somewhere in NodeSaveTest:
- Grammar: "Checks *whether* " would be better. [I see this a lot in this patch.]
- Spelling: ids -> IDs
g)
Needs "that" after Tests.
h)
I can understand removing the @see for node_filter_form() since it's in the first line, but why did you decide none of those others was relevant? I would generally tend to advise leaving @see references if someone thought they were relevant, unless the functions no longer exist. This applies to a couple of other blocks in the patch as well.
i) Hook docs in node.api.php have been changed to wrong verb tense -- see
http://drupal.org/node/1354#hooks
j) I'm not fond (grammatically) of this change in node.install:
This makes it redundant -- rename name ?!?
k)
- First line change is not accurate - the previous doc was.
- I don't think the $variables['elements'] documentation is accurate, but I'm not sure. I would also document the 'node' array element as "The node object", since it's specifically the node being displayed, not just any old node object. Also, before "e.g.", you need a ;
l) I liked this better in the original (list format):
m) The * got lost here:
n) Needs to start with a verb:
and why did you make this node_access? It was fine as it was (prose, not referring to a particular function).
o)
typo on that last line. :)
Comment #89
Lars Toomre CreditAttribution: Lars Toomre commentedThanks for the detailed review @jhodgdon. Given that others have not bothered to review #86 before your detailed committer review, I will leave this patch for others to drive home.
Neither @jdodgdon nor I should be left to correct/review whether a module conforms with D8 documentation standards.
Comment #90
jhodgdonThat is your choice Lars. I am quite happy to review/commit these API docs cleanup patches.
Comment #91
Albert Volkman CreditAttribution: Albert Volkman commentedHere's my attempt to resolve the issues presented by @jhodgdon. I cleaned reworked @Lars Toomre patch to apply cleanly to create an interdiff.
Comment #92
jhodgdonEek! I noticed #1824016: Confusing name for test method in NodeBlockTest but that is a separate issue.
So this patch is almost perfect! I reviewed it from the top and saw these mostly minor remaining clean-ups needed:
a)
- verify -> verifies in first block
- second one is missing . at end
b)
Two spaces after . in first line, and if you are fixing that, could you also fix the grammar? which -> that (or who)
c) in node.api.php
This kind of change is not really appropriate. The order of the @see lines was more logical before. We don't have a standard saying they should be alphabetical. We also don't have a standard saying we need a blank line between @see and @ingroup.
So we need all of these types of changes removed from the patch -- they are not helping the documentation conform to our standards and in some cases (such as this one), they are making the documentation less logical. That will make the patch smaller and easier to review too.
d)
I don't think this change in node.api.php is a good idea either. Lists are easier to read.
e) in node.module, docs for _node_query_node_access_alter():
This is not accurate -- this function does not prepare any functions. It actually alters node access queries.
f) node.module, docs for _node_access_write_grants():
Um. So this parameter is now a Node object, so the info about nid is not relevant. Also we shouldn't use $node in prose like that. The docs should just say "The node whose grants are being written." and nothing else.
g) node.module
This @ingroup is not appropriate. That topic is for the base batch operations functions:
http://api.drupal.org/api/drupal/includes!form.inc/group/batch/7
Comment #93
Albert Volkman CreditAttribution: Albert Volkman commentedHere ya go.
Comment #94
jhodgdonA couple of requests:
a) Could you also take out @ingroup batch for node_mass_update() in node.admin.inc? [see #92 g]
b) Why are all these include file changes in a node module patch? Please remove them and upload a patch that just addresses node module changes for this issue. Sorry I didn't notice that before.
Comment #95
Albert Volkman CreditAttribution: Albert Volkman commented@ingroup removed from node_mass_update()
Non-node module updates removed (looks like they were introduced back in #84
Comment #97
jhodgdon#95: node_docs-1313980-95.patch queued for re-testing.
Comment #99
jhodgdonUh oh. The first time, a couple of unrelated tests failed, so I clicked "re-test". Now it looks like it needs a re-roll.
Comment #100
Albert Volkman CreditAttribution: Albert Volkman commentedRe-rolled
Comment #101
jhodgdonThanks! This is fairly close...
One thing I noticed is that in the added/fixed docs one-line summaries, especially in the test classes, this patch has a lot of kind of awkward wording, such as:
- Tests if field languages are correctly set ==> if -> whether
- This test class requires tags taxonomy field. ==> needs the and maybe quotes
- Tests the rebuilding of node access permissions table. ==> Tests rebuilding the node access permissions table.
- Note hook_node_access_records() is covered in another test class. ==> needs that
- Tests the creation of and saving a node entity. ==> Tests creating and saving a node.
- Checks changing node authored by fields.
etc. I am not thrilled about committing it as-is... if someone wants to clean it up I would be very happy!
Then there are a couple of more specific minor things that should be fixed:
a) misuse of i.e. (should be "for example" instead) in hook_node_types:
and a little bit down, a double ..:
b) And this kind of change is really totally unnecessary and just adds clutter:
If this argument is not used, why would we even care what it defaults to? I would just remove all the hunks in the patch that just add "Defaults to..." because this information is in the function signature anyway and doesn't add much, if anything, to the documentation (just makes one more thing to maintain).
Comment #102
Albert Volkman CreditAttribution: Albert Volkman commentedI completed the items specifically noted by @jhodgdon, but didn't go any further.
Comment #103
jhodgdonThose changes look good, thanks!
I scanned through the patch one more time, and I think the rest of the wording is good enough.
Uh oh! I did see one thing that does need to be fixed this time through. Verb tense is different for hook definitions, so this change in node.api.php is not right:
Quick reroll for that and we can call this one RTBC. Thanks!
Comment #104
Albert Volkman CreditAttribution: Albert Volkman commentedDone!
Comment #105
jhodgdonGreat! There are sprints galore happening this weekend for BADCamp, so I'm waiting on commits until next week (each commit adds a load to the bot, and that slows down patch reviews during sprints), but I'll get it in soon.
Comment #106
jhodgdonGracious! This issue has been going on a LONG time and many people have contributed! I just committed this patch to 8.x. YIPPEEEEE! Thanks to all!
I would now propose a backport: just see what you can get to apply and we'll get that committed. You can try applying the various Test*.php bits to node.test and that might work... or might not. Anyway, let's see what we can do for 7.x without too much work from this last patch.
And if anyone has followups for node module, file a new issue! This one is long enough. :)
Comment #107
nod_Hi, don't backport changes to the JS files please, JS files are not commented on purpose for D7.
And can you make sure to tag issues with the JavaScript tag is there is a patch touching any JS file? I can't keep track otherwise.
Thanks.
Comment #108
jhodgdonRE #107 - I was not aware that JS files should not have documentation at the top for Drupal 7 -- where is this standard documented and what is the reason?
Comment #109
nod_Well it's… undocumented that they shouldn't have documentation. Look at drupal.js on D7, no file header.
The reason is filesize. We're not minifing files (and striping out comments, etc.) since JS doc isn't integrated with api.d.o it ends up being more harmful than helpful. Hence, no @file for D7 JS.
Comment #110
jhodgdonIn that case, our coding/docs standards currently say that all files should have an @file in them... If you want to amend that, please file an issue in Drupal Core with tag 'coding standards' rather than discussing it here. Thanks!
Comment #111
Albert Volkman CreditAttribution: Albert Volkman commentedFirst pass at this.
Comment #112
jhodgdonThanks for the patch!
A couple of minor things to fix:
a) See #107 - one of the JS maintainers has asked us not to put @file docblocks into *.js files for Drupal 7. So can we (sigh) take those out?
b) This needs a blank line before the @ingroup:
The rest of the patch looks great! So if we can get a quick reroll for these two issues, I can get this commited and we can put this issue to rest. Thanks!
Comment #113
Albert Volkman CreditAttribution: Albert Volkman commentedSo I started perusing the D7 node docs a bit more in depth and noticed quite a few omissions (missing docs, inconsistencies, etc). Not sure if that should be filed as a separate issue. So, to keep things on track, I've included a patch resolving the two issues in #112 (with the "a" patch and interdiff), then a second patch with the additional fixes (also including fixes from #112).
Comment #114
jhodgdonRegarding the follow-ups: are they problems in Drupal 8.x too? If so, let's have a follow-up issue to fix them in 8/7. If not, I will review the (b) patch carefully and get it committed here. Thanks!
Comment #115
Albert Volkman CreditAttribution: Albert Volkman commentedI actually copied most of them from D8.
Comment #116
jhodgdonOh cool. I'll give it a review then and get it committed, probably Monday. Thanks!
Comment #117
Albert Volkman CreditAttribution: Albert Volkman commentedBackpedaling a bit to D8. I saw this line-
http://drupalcode.org/project/drupal.git/blob/refs/heads/8.x:/core/modul...
Which references this function-
http://api.drupal.org/api/drupal/core%21modules%21node%21node.module/fun...
Which doesn't actually exist in D8. Its been renamed to-
http://api.drupal.org/api/drupal/core%21modules%21node%21node.module/fun...
Why are both showing on a.d.o (and we should probably fix that reference)?
Comment #118
jhodgdonRE #117 - yes, fix the reference.
As far as why api.d.o is not updated, ... known issue... api.d.o is having problems lately and d8 on there is just way out of date. We're working on it... should be fixed $soon (but I'm not sure of the exact value of $soon). Sorry about that!
Comment #119
Albert Volkman CreditAttribution: Albert Volkman commentedHere's a patch for #117.
Comment #120
jhodgdonThanks! I'll get that one committed, and then go back and review (and hopefully commit) the 7.x patch in #113.
Comment #121
jhodgdonPatch in #119 is committed to 8.x. Now back to 7.x for the patch in #112(b), which I'll review when I can. Thanks again!
Comment #122
jhodgdonI went back to the patch in #112(b) and started from the top for a complete review. It all looks good except these lines:
a) node.api.php
entity_load() is duplicated here.
b) node.module
Typo in the first line !n instead of An
c) node.pages.inc
Shouldn't this be documented as a standard form validation function?
d) node.pages.inc
And here is the submit handler for (c)...
e) There seem to be several functions in node.pages.inc that didn't get updates to their first-line verb tense.
Let's at least fix up a-d, and as a bonus e as well, and get this done! Thanks!!
Comment #123
Albert Volkman CreditAttribution: Albert Volkman commentedHere ya go. Found a couple more too. Interdiff'd against 113b.
Comment #124
jhodgdonExcellent! I'll get this committed when it turns green. :)
Comment #125
jhodgdonCommitted to 7.x, and there was much rejoicing!!!!!
Comment #127
jenlamptonOne small issue, the docs in the D7 version now say "@param Drupal\node\Node $node" but I think that was leftover from the D8 patch.
Comment #128
jenlamptonWhoops, maybe not. Looks like that was fixed over in https://drupal.org/node/2117601