CommentFileSizeAuthor
#123 node_docs-1313980-123.patch74.27 KBAlbert Volkman
#123 interdiff.txt4.46 KBAlbert Volkman
#119 node_docs-1313980-119.patch512 bytesAlbert Volkman
#113 node_docs-1313980-113a.patch56.36 KBAlbert Volkman
#113 interdiffa.txt890 bytesAlbert Volkman
#113 node_docs-1313980-113b.patch72.93 KBAlbert Volkman
#113 interdiffb.txt19.31 KBAlbert Volkman
#111 node_docs-1313980-111.patch56.92 KBAlbert Volkman
#104 node_docs-1313980-104.patch66.8 KBAlbert Volkman
#104 interdiff.txt492 bytesAlbert Volkman
#102 node_docs-1313980-102.patch67.1 KBAlbert Volkman
#102 interdiff.txt8.52 KBAlbert Volkman
#100 node_docs-1313980-100.patch70.19 KBAlbert Volkman
#95 node_docs-1313980-95.patch70.34 KBAlbert Volkman
#95 interdiff.txt7.02 KBAlbert Volkman
#93 node_docs-1313980-93.patch76.79 KBAlbert Volkman
#93 interdiff.txt9.82 KBAlbert Volkman
#91 1313980-91-node-docs.patch80.97 KBAlbert Volkman
#91 interdiff.txt27.47 KBAlbert Volkman
#86 1313980-86-node-docs.patch97.54 KBLars Toomre
#84 1313980-84-node-docs.patch97.58 KBLars Toomre
#78 1313980-78-node-docs.patch22.75 KBLars Toomre
#78 interdiff-1313980-76-78.txt8.83 KBLars Toomre
#76 1313980-75-node-docs.patch22.74 KBLars Toomre
#75 1313980-75-node-docs.patch40.13 KBLars Toomre
#71 1313980-71-node-docs.patch22.36 KBLars Toomre
#65 node_module_docs-1313980-d7-65.patch53.13 KBAlbert Volkman
#65 interdiff.txt2.6 KBAlbert Volkman
#63 node_module_docs-1313980-d7-63.patch53.1 KBAlbert Volkman
#60 node_module_docs-1313980-d7-60.patch59.22 KBAlbert Volkman
#60 interdiff.txt2.17 KBAlbert Volkman
#57 node_module_docs-1313980-d7-57.patch59.22 KBAlbert Volkman
#57 interdiff.txt2.17 KBAlbert Volkman
#52 node_module_docs-1313980-d8-52.patch2.24 KBAlbert Volkman
#50 node_module_docs-1313980-d8-49.patch1.97 KBAlbert Volkman
#47 node_module_docs-1313980-d7-46.patch58.85 KBAlbert Volkman
#47 interdiff.txt6.84 KBAlbert Volkman
#44 node_module_docs-1313980-d7-44.patch60.44 KBAlbert Volkman
#35 node_module_docs-1313980-35.patch63.72 KBjn2
#28 node_module_docs-1313980-28.patch61.02 KBjn2
#23 node_module_docs-1313980-23.patch56.82 KBjn2
#20 node_module_docs-1313980-20.patch56.55 KBjn2
#15 node_module_docs-1313980-15.patch56.64 KBjn2
#12 node_module_docs-1313980-12.patch56.07 KBjn2
#5 node_module_docs-1313980-5.patch55.63 KBjn2
#3 node_module_docs-1313980-3.patch57.44 KBjn2
#1 node_module_docs-1313980-1.patch56.63 KBjn2
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jn2’s picture

Status: Active » Needs review
FileSize
56.63 KB

Here's the patch.

xjm’s picture

Status: Needs review » Needs work

Hey @jn2, I'm working on the sprint as well, so I thought I'd help review your patch. Here are some thing I noticed:

+++ b/modules/node/node.admin.incundefined
@@ -14,7 +19,9 @@ function node_configure_rebuild_confirm() {
+ * Form submission handler for wipe confirmation.

+++ b/modules/node/node.moduleundefined
@@ -2319,7 +2393,7 @@ function node_form_block_admin_configure_alter(&$form, &$form_state) {
+ * Form submission handler for block configuration form.

@@ -2351,7 +2426,7 @@ function node_form_block_custom_block_delete_alter(&$form, &$form_state) {
+ * Form submission handler for custom block delete form.

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()."

+++ b/modules/node/node.admin.incundefined
@@ -133,7 +143,9 @@ function node_build_filter_query(SelectQueryInterface $query) {
- * Return form for node administration filters.
+ * Returns form for node administration filters.
+ *
+ * @see node_filter_form_submit()

@@ -208,7 +220,9 @@ function node_filter_form() {
- * Process result from node administration filter form.
+ * Processes result from node administration filter form.
+ *
+ * @see node_filter_form()

@@ -353,7 +378,11 @@ function node_admin_content($form, $form_state) {
- * Form builder: Builds the node administration overview.
+ * Helper function that builds the node administration overview form.
+ *

@@ -521,9 +550,9 @@ function node_admin_nodes() {
- * Validate node_admin_nodes form submissions.
+ * Validates node_admin_nodes() form submissions.

@@ -534,9 +563,9 @@ function node_admin_nodes_validate($form, &$form_state) {
- * Process node_admin_nodes form submissions.
+ * Processes node_admin_nodes() form submissions.

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.

+++ b/modules/node/node.admin.incundefined
@@ -237,7 +251,7 @@ function node_filter_form_submit($form, &$form_state) {
+ * Makes mass update of nodes, changing all nodes in the $nodes array
  * to update them with the field values in $updates.

We should probably reword this so that the important points fit on one line, and the additional details are deferred to another paragraph.

+++ b/modules/node/node.admin.incundefined
@@ -279,7 +295,12 @@ function node_mass_update($nodes, $updates) {
+ * Helper function for node_mass_update().

+++ b/modules/node/node.installundefined
@@ -444,7 +444,7 @@ function node_install() {
+ * Utility function: Fetches the node types directly from the database.

+++ b/modules/node/node.moduleundefined
@@ -1157,9 +1177,19 @@ function node_save($node) {
+ * Helper function that saves a revision with the uid of the current user.

+++ b/modules/node/node.pages.incundefined
@@ -323,7 +363,15 @@ function node_form_build_preview($form, &$form_state) {
+ * Helper function that generates a node preview.

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.

+++ b/modules/node/node.admin.incundefined
@@ -293,7 +314,7 @@ function _node_mass_update_helper($nid, $updates) {
+ * Batch operation for node_mass_update().

Needs a verb, I think.

+++ b/modules/node/node.moduleundefined
@@ -2,60 +2,72 @@
- * Node is not published.
+ * Denotes node is not published.

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.

+++ b/modules/node/node.moduleundefined
@@ -3428,7 +3518,7 @@ function node_access_needs_rebuild($rebuild = NULL) {
+ * Rebuilds the node access database. This is occasionally needed by modules
  * that make system-wide changes to access levels.

We should probably make this one line (the first sentence) and put the additional info in a separate paragraph.

+++ b/modules/node/node.pages.incundefined
@@ -303,7 +337,9 @@ function node_form($form, &$form_state, $node) {
+ * Button submission handler for the 'Delete' button on the node form.

@@ -315,7 +351,11 @@ function node_form_delete_submit($form, &$form_state) {
+ * Button submission handler for the 'Preview' button on the node form.

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. :)

jn2’s picture

Status: Needs work » Needs review
FileSize
57.44 KB

Thanks for the review, xjm. Here's a new patch that should address all your comments.

xjm’s picture

Status: Needs review » Needs work

Thanks @jn2. I read through your updated patch and found a few more small things:

+++ b/modules/node/content_types.incundefined
@@ -429,7 +448,9 @@ function node_type_delete_confirm($form, &$form_state, $type) {
- * Process content type delete confirm submissions.
+ * Processes content type delete confirm submissions.

+++ b/modules/node/node.pages.incundefined
@@ -480,7 +540,7 @@ function node_delete_confirm($form, &$form_state, $node) {
+ * Executes node deletion.

Here's another couple form submission handlers.

+++ b/modules/node/node.admin.incundefined
@@ -279,7 +290,14 @@ function node_mass_update($nodes, $updates) {
+ * Updates individual nodes.

Maybe "Updates individual nodes with the supplied values" or something along those lines?

+++ b/modules/node/node.moduleundefined
@@ -2814,7 +2898,7 @@ function node_search_validate($form, &$form_state) {
- * Determine whether the current user may perform the given operation on the
+ * Determines whether the current user may perform the given operation on the
  * specified node.

@@ -3399,7 +3486,7 @@ function _node_access_write_grants($node, $grants, $realm = NULL, $delete = TRUE
- * Flag / unflag the node access grants for rebuilding, or read the current
+ * Toggles a flag for rebuilding the node access grants, or reads the current
  * value of the flag.

Looks like these summaries are not one line.

+++ b/modules/node/node.moduleundefined
@@ -3672,6 +3760,11 @@ function node_action_info() {
+ * @param $context
+ *   (optional) Allows context to be specified.

@@ -3682,6 +3775,11 @@ function node_publish_action($node, $context = array()) {
+ * @param $context
+ *   (optional) Allows context to be specified.

@@ -3692,6 +3790,11 @@ function node_unpublish_action($node, $context = array()) {
+ * @param $context
+ *   (optional) Allows context to be specified.

@@ -3702,6 +3805,11 @@ function node_make_sticky_action($node, $context = array()) {
+ * @param $context
+ *   (optional) Allows context to be specified.

@@ -3712,6 +3820,11 @@ function node_make_unsticky_action($node, $context = array()) {
+ * @param $context
+ *   (optional) Allows context to be specified.

@@ -3722,6 +3835,11 @@ function node_promote_action($node, $context = array()) {
+ * @param $context
+ *   (optional) Allows context to be specified.

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?

+++ b/modules/node/node.moduleundefined
@@ -3758,6 +3883,10 @@ function node_assign_owner_action($node, $context) {
  * Generates the settings form for node_assign_owner_action().

+++ b/modules/node/node.pages.incundefined
@@ -78,7 +109,9 @@ function node_form_validate($form, &$form_state) {
+ * Generates the node add/edit form array.

I think these need to be switched over to the standard for form builders.

+++ b/modules/node/node.moduleundefined
@@ -3903,6 +4035,9 @@ function node_modules_enabled($modules) {
+   * Implements DrupalDefaultEntityController::attachLoad().

@@ -3925,6 +4060,9 @@ class NodeController extends DrupalDefaultEntityController {
+   * Implements DrupalDefaultEntityController::buildQuery().

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.

+++ b/modules/node/node.pages.incundefined
@@ -27,12 +42,15 @@ function node_add_page() {
+ * @return
+ *   HTML for a list of available node types for node creation.
+ *

@@ -363,12 +411,15 @@ function node_preview($node) {
+ * @return
+ *   HTML for a node preview for display during node creation and editing.
+ *

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. :)

+++ b/modules/node/node.pages.incundefined
@@ -303,7 +337,9 @@ function node_form($form, &$form_state, $node) {
+ * Form submission handler for the 'Delete' button on the node form.

@@ -315,7 +351,11 @@ function node_form_delete_submit($form, &$form_state) {
+ * Form submission handler for the 'Preview' button on the node form.

Maybe these can say "for the 'Foo' button of node_form()" rather than "on the node form" like the others you changed already.

jn2’s picture

Status: Needs review » Needs work
FileSize
55.63 KB

@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!

jn2’s picture

Status: Needs work » Needs review
xjm’s picture

Status: Needs work » Needs review
+++ b/modules/node/node.admin.incundefined
@@ -237,18 +247,19 @@ function node_filter_form_submit($form, &$form_state) {
+ * @see batch

Oops, 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.

jhodgdon’s picture

Status: Needs review » Needs work

Looks pretty good, thanks! And I'm not sure who these "docs folks" are -- aren't we all? :)

A couple of things to fix:
a)

 /**
  * Helper function for teaser length choices.
+ *
+ * @return
+ *   String containing the length of the teaser in characters.
  */
 function _node_characters($length) {

Needs verbification of first line.

b)

 /**
+ * Page callback: Deletes a single content type.
...
+ * @return
+ *   Form array for delete confirmation form.
+ *
+ * @see node_type_delete_confirm_submit()
  */
 function node_type_delete_confirm($form, &$form_state, $type) {

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):

  *   A list of channel elements can be found at the @link http://cyber.law.harvard.edu/rss/rss.html RSS 2.0 Specification. @endlink
jn2’s picture

@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.

jhodgdon’s picture

I'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. :)

jhodgdon’s picture

OK, 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.

jn2’s picture

Status: Needs work » Needs review
FileSize
56.07 KB

@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.

xjm’s picture

Alright, 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.

+++ b/modules/node/node.admin.incundefined
@@ -279,7 +290,14 @@ function node_mass_update($nodes, $updates) {
+ * Updates individual nodes if fewer than 10.

Fewer than 10 what? :) How about: "Updates individual nodes when fewer than 10 are queued."

+++ b/modules/node/node.admin.incundefined
@@ -293,7 +311,7 @@ function _node_mass_update_helper($nid, $updates) {
+ * Executes batch operation for node_mass_update().

@@ -324,7 +342,7 @@ function _node_mass_update_batch_process($nodes, $updates, &$context) {
+ * Reports 'finished' status of batch operation for node_mass_update().

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 we could add article here as well.

I think these should fit 80 chars:

Executes a batch operation for node_mass_update().

Reports the 'finished' status of a batch operation for node_mass_update().

+++ b/modules/node/node.moduleundefined
@@ -897,26 +911,29 @@ function node_invoke($node, $hook, $a2 = NULL, $a3 = NULL, $a4 = NULL) {
- *   Whether to reset the internal node_load cache.
+ *   (optional) Whether to reset the internal node_load cache.
...
- *   Whether to reset the node_load_multiple cache.
+ *   (optional) Whether to reset the node_load_multiple cache.

While we're changing these lines, I think it should be node_load() and node_load_multiple() (with parens)?

+++ b/modules/node/node.moduleundefined
@@ -1413,10 +1452,13 @@ function node_show($node, $message = FALSE) {
+ *   The ID of the node if full page view, otherwise FALSE.

How about, "The ID of the node if this is a full page view, or FALSE otherwise."

+++ b/modules/node/node.moduleundefined
@@ -1848,6 +1903,12 @@ function _node_revision_access($node, $op = 'view') {
+ *   TRUE if user has permission, otherwise FALSE.

"TRUE if the user has permission to access the form, or FALSE otherwise."

+++ b/modules/node/node.moduleundefined
@@ -2436,13 +2509,14 @@ function node_block_list_alter(&$blocks) {
+ *   (optional) An associative array containing title, link, description and
+ *   other keys, to be parsed by format_rss_channel() and format_xml_elements().

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?

+++ b/modules/node/node.moduleundefined
@@ -2825,10 +2909,11 @@ function node_search_validate($form, &$form_state) {
  *   The node object on which the operation is to be performed, or node type
- *   (e.g. 'forum') for "create" operation.
+ *   (e.g., 'forum') for "create" operation.

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.)

+++ b/modules/node/node.moduleundefined
@@ -3428,26 +3515,26 @@ function node_access_needs_rebuild($rebuild = NULL) {
  *   hook_update_N and any form submit handler are safe contexts to use the
  *   'batch mode'. Less decidable cases (such as calls from hook_user,
  *   hook_taxonomy, etc...) might consider using the non-batch mode.

We could add parens here for hook_update_N(), hook_user(), and hook_taxonomy().

+++ b/modules/node/node.pages.incundefined
@@ -55,7 +71,17 @@ function theme_node_add_list($variables) {
+ * Path: node/add/' . $type_url_str

Hmm, bit weird with the one floating quotation mark and the variable here. Maybe just "node/add/%"?

+++ b/modules/node/node.pages.incundefined
@@ -464,7 +515,11 @@ function node_form_submit_build_node($form, &$form_state) {
+ * Page callback: Asks for confirmation of node deletion.

I think this is another one of those hybrid menu/form constructor dealies.

Thanks so much for all your patience with the rerolls. :)

xjm’s picture

Status: Needs review » Needs work

Note 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.

jn2’s picture

Status: Needs work » Needs review
FileSize
56.64 KB

Here'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:

"TRUE if the user has permission to access the form, or FALSE otherwise."

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.

Hmm, bit weird with the one floating quotation mark and the variable here. Maybe just "node/add/%"?

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.

jhodgdon’s picture

Status: Needs review » Needs work

I 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)

 /**
- * Helper function for teaser length choices.
+ * Returns teaser length choices.
+ *
+ * @return
+ *   String containing the length of the teaser in characters.
  */

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)

+ * Applies filters for node administration filters based on session.

This looks a bit redundant - applies filters for filters?

d)

/**
- * Make mass update of nodes, changing all nodes in the $nodes array
- * to update them with the field values in $updates.
+ * Updates all nodes in the $nodes array with the field values in $updates.

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)

- *   The user object to perform the access check operation on.
+ *   A user object for which the access check operation is to be performed.

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)

  /**
+ * Denotes node is not published.

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)

+ *   (optional) An object or array containing values to override the defaults.
+ *   See hook_node_info() for details on what the array elements mean.
  *
  * @return
  *   A node type object, with missing values in $info set to their defaults.
+ *
+ * @see hook_node_info()

This doesn't really need the @see. The reference is made in the text above, with context to explain why.

i)

+ * Saves a revision with the uid of the current user.

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)

+ *   FALSE if would delete the current revision; TRUE if deletion successful.

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?

jn2’s picture

@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?

xjm’s picture

[milankovitch:drupal | Wed 20:32:41] $ grep -r "_node_characters" *
core/modules/node/content_types.inc:function _node_characters($length) {

So 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.

jhodgdon’s picture

Good detective work xjm - not touching the docblock and filing a follow-up issue sounds like the best course of action. :)

jn2’s picture

Status: Needs work » Needs review
FileSize
56.55 KB

Here's the patch with revisions from #16. Also went through everything again looking for overly terse language.

jhodgdon’s picture

Status: Needs review » Needs work

This is looking really good now, thanks!

I found one small thing:

+ *   (optional) Set to TRUE to process in 'batch' mode, spawning processing over
+ *   several HTTP requests (thus avoiding the risk of PHP timeout if the site
+ *   has a large number of nodes).
+ *   hook_update_N() and any form submit handler are safe contexts to use the
+ *   'batch mode'. Less decidable cases (such as calls from hook_user(),
+ *   hook_taxonomy(), etc.) might consider using the non-batch mode.

Paragraph wrapping -- 4th line should be wrapped with 3rd etc.

If you're going to fix that, you could also fix this:

+ *   Array with the following elements:
+ *   - 'owner_uid': User ID assigned to the node, if any.

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...

xjm’s picture

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...

Yeah, I'd agree that this is a good idea.

jn2’s picture

Here'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.

jn2’s picture

Status: Needs work » Needs review
xjm’s picture

I 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.

xjm’s picture

Alright, here are all the form "families" I found:

  • node_revision*
    node.pages.inc:function node_revision_revert_confirm($form, $form_state, $node_revision) {
    node.pages.inc:function node_revision_revert_confirm_submit($form, &$form_state) {
    node.pages.inc:function node_revision_delete_confirm($form, $form_state, $node_revision) {
    node.pages.inc:function node_revision_delete_confirm_submit($form, &$form_state) {
    
  • node_delete*
    node.pages.inc:function node_delete_confirm($form, &$form_state, $node) {
    node.pages.inc:function node_delete_confirm_submit($form, &$form_state) {
    
  • node_form*
    (some of these are hook implementations or alters and not actually related to one single form)
    node.module:function node_form_block_add_block_form_alter(&$form, &$form_state) {
    node.module:function node_form_block_admin_configure_alter(&$form, &$form_state) {
    node.module:function node_form_block_admin_configure_submit($form, &$form_state) {
    node.module:function node_form_block_custom_block_delete_alter(&$form, &$form_state) {
    node.module:function node_form_block_custom_block_delete_submit($form, &$form_state) {
    node.module:function node_form_search_form_alter(&$form, $form_state) {
    node.pages.inc:function node_form_validate($form, &$form_state) {
    node.pages.inc:function node_form($form, &$form_state, $node) {
    node.pages.inc:function node_form_delete_submit($form, &$form_state) {
    node.pages.inc:function node_form_build_preview($form, &$form_state) {
    node.pages.inc:function node_form_submit($form, &$form_state) {
    node.pages.inc:function node_form_submit_build_node($form, &$form_state) {
    
  • node_assign_owner_action*
    The actions aren't themselves normal form builders but should likely have @see relationships with their validators if they don't already.
    node.module:function node_assign_owner_action($node, $context) {
    node.module:function node_assign_owner_action_form($context) {
    node.module:function node_assign_owner_action_validate($form, $form_state) {
    node.module:function node_assign_owner_action_submit($form, $form_state) {
  • node_unpublish_by_keyword_action*
    node.module:function node_unpublish_by_keyword_action_form($context) {
    node.module:function node_unpublish_by_keyword_action_submit($form, $form_state) {
    node.module:function node_unpublish_by_keyword_action($node, $context) {
    
  • node_admin*
    node_admin_nodes() actually just returns the form object to node_admin_content(), which is the actual constructor.
    node.admin.inc:function node_admin_content($form, $form_state) {
    node.admin.inc:function node_admin_nodes() {
    node.admin.inc:function node_admin_nodes_validate($form, &$form_state) {
    node.admin.inc:function node_admin_nodes_submit($form, &$form_state) {
    
  • node_filter*
    node_filter_form() is similarly called by node_admin_content().
    node.admin.inc:function node_filter_form() {
    node.admin.inc:function node_filter_form_submit($form, &$form_state) {
    
  • node_configure_rebuild*
    node.admin.inc:function node_configure_rebuild_confirm() {
    node.admin.inc:function node_configure_rebuild_confirm_submit($form, &$form_state) {
    
  • node_type*
    content_types.inc:function node_type_form($form, &$form_state, $type = NULL) {
    content_types.inc:function node_type_form_validate($form, &$form_state) {
    content_types.inc:function node_type_form_submit($form, &$form_state) {
    content_types.inc:function node_type_delete_confirm($form, &$form_state, $type) {
    content_types.inc:function node_type_delete_confirm_submit($form, &$form_state) {
  • node_multiple_delete*
    node.admin.inc:function node_multiple_delete_confirm($form, &$form_state, $nodes) {
    node.admin.inc:function node_multiple_delete_confirm_submit($form, &$form_state) {
    

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, and node_search_validate() is now a validation handler for node_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 be hook_form_FORM_ID_alter() (with an underscore)?

Edit 2: Done finally after some distraction.

jhodgdon’s picture

Status: Needs review » Needs work

Yes 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.

jn2’s picture

Status: Needs work » Needs review
FileSize
61.02 KB

Here's the patch.

Edited: This isn't quite done yet.

xjm’s picture

jn2’s picture

@xjm #29

I was already doing that. I read that as part of the original specification, although maybe it wasn't.

xjm’s picture

@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.)

jn2’s picture

I see. I didn't read it closely enough. So I'll make those changes.

xjm’s picture

Well, 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.

jhodgdon’s picture

Status: Needs review » Needs work

Since this patch isn't done (as per author of patch), moving back to needs work.

jn2’s picture

Here's a patch that should address all the issues from #26.

jn2’s picture

Status: Needs work » Needs review
xjm’s picture

Status: Needs review » Reviewed & tested by the community

63k, 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. :)

catch’s picture

#35: node_module_docs-1313980-35.patch queued for re-testing.

catch’s picture

Status: Reviewed & tested by the community » Fixed

That's a lot of clean-up!

Committed/pushed to 8.x.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

xjm’s picture

Version: 8.x-dev » 7.x-dev
Status: Closed (fixed) » Patch (to be ported)

This needs to be backported; however, it will not be an easy backport. :)

xjm’s picture

Assigned: jn2 » Unassigned
Albert Volkman’s picture

Assigned: Unassigned » Albert Volkman

I'll take it :)

Albert Volkman’s picture

Status: Patch (to be ported) » Needs review
FileSize
60.44 KB

D7 backport.

xjm’s picture

Status: Needs review » Needs work

Found 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.)

+++ b/modules/node/node.api.phpundefined
@@ -574,19 +568,19 @@ function hook_node_load($nodes, $types) {
+ * @param object|string $node
...
+ * @param object $account
...
+ * @return integer

Variable data types for @param and @return should not be added to D7.

+++ b/modules/node/node.moduleundefined
@@ -1546,7 +1589,7 @@ function node_permission() {
- * Gather the rankings from the the hook_ranking implementations.
+ * Gathers the rankings from the the hook_ranking implementations.

I think hook_ranking() needs parens. Is this in D8?

+++ b/modules/node/node.moduleundefined
@@ -1862,7 +1906,7 @@ function _node_revision_access($node, $op = 'view', $account = NULL) {
-    // and the vid of the current revision differ, then we already have two
+    // and the vid of the current revision differs, then we already have two

This change is grammatically incorrect and should be reverted. Was this in the D8 patch?

+++ b/modules/node/node.moduleundefined
@@ -2096,14 +2148,24 @@ function node_menu_local_tasks_alter(&$data, $router_item, $root_path) {
+ * Title callback: Displays the node's title.
+ *
+ * @param $node
+ *   The node object.
+ *
+ * @see node_menu()

The summary change here is probably okay for backport, but not the @see.

+++ b/modules/node/node.moduleundefined
@@ -2307,7 +2369,7 @@ function theme_node_recent_content($variables) {
+ * Implements hook_form_FORM_ID_alter().

@@ -2318,7 +2380,7 @@ function node_form_block_add_block_form_alter(&$form, &$form_state) {
+ * Implements hook_form_FORM_ID_alter().

@@ -2369,10 +2429,11 @@ function node_form_block_admin_configure_submit($form, &$form_state) {
- * Implements hook_form_FORMID_alter().
+ * Implements hook_form_FORM_ID_alter().
...
  * @see block_custom_block_delete()

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.

+++ b/modules/node/node.moduleundefined
@@ -2459,20 +2518,22 @@ function node_block_list_alter(&$blocks) {
- * Generates and prints an RSS feed.
+ * Page callback: Generates and prints an RSS feed.

I think this change is also probably not supposed to be backported (sinc it's a "new" standard for D8).

+++ b/modules/node/node.moduleundefined
@@ -2574,7 +2635,9 @@ function node_view_multiple($nodes, $view_mode = 'teaser', $weight = 0, $langcod
+ * @see node_menu()

@@ -2624,7 +2687,12 @@ function node_page_default() {
+ * @see node_menu()

The @see module_menu() need to go away for the backport, I think.

+++ b/modules/node/node.moduleundefined
@@ -3777,7 +3889,16 @@ function node_assign_owner_action($node, $context) {
+ * Form constructor for the settings form for node_assign_owner_action().
...
+ * @ingroup form

Shouldn't this be @ingroup forms? Did it go into D8 like this as well?

jhodgdon’s picture

Really, we can't have @see node_menu() in there in D7? I think those can stay.

Albert Volkman’s picture

Status: Needs work » Needs review
FileSize
6.84 KB
58.85 KB

I 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.

jhodgdon’s picture

RE 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().

jhodgdon’s picture

Status: Needs review » Needs work

had forgotten to change the status over the @see stuff.

Albert Volkman’s picture

Version: 7.x-dev » 8.x-dev
Status: Needs work » Needs review
FileSize
1.97 KB

Here's my stab at fixing D8.

jhodgdon’s picture

Status: Needs review » Needs work

Um...

@@ -2421,18 +2421,16 @@ function theme_node_recent_content($variables) {
 }
 
 /**
- * Implements hook_form_FORM_ID_alter().
+ * Implements hook_form_FORM_ID_alter() for block_custom_block_delete().
  *
  * Adds node-type specific visibility options to add block form.
- *
- * @see block_add_block_form()
  */
 function node_form_block_add_block_form_alter(&$form, &$form_state) {

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.

Albert Volkman’s picture

Wow, absolutely no excuse for that. My apologies.

jhodgdon’s picture

Status: Needs work » Needs review

bot trigger

jhodgdon’s picture

We 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.

xjm’s picture

Status: Needs review » Reviewed & tested by the community

#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... :)

jhodgdon’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Actually, #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.

Albert Volkman’s picture

Status: Patch (to be ported) » Needs review
FileSize
2.17 KB
59.22 KB

Updated patch with an interdiff against #52.

Albert Volkman’s picture

Hrm... is it stuck?

jhodgdon’s picture

Looks stuck. Try uploading the patch again.

Albert Volkman’s picture

xjm’s picture

jhodgdon’s picture

Status: Needs review » Needs work

Sorry 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:

-  // Reset internal static cache of _node_types_build(), forces to rebuild the
-  // node type information.
+  // Resets the internal static cache of _node_types_build() and forces a
+  // rebuild of the node type information.

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:

 /**
- * Generate the node add/edit form array.
+ * Form constructor for the node add/edit form.
+ *
+ * @see node_form_delete_submit()
+ * @see node_form_build_preview()
+ * @see node_form_validate()
+ * @see node_form_submit()
+ * @see node_form_submit_build_node()
+ * @ingroup forms
  */
 function node_form($form, &$form_state, $node) {

c) This is in node.pages.inc:

+ * Form constructor for node deletion confirmation form.
+ *
+ * @see node_menu()
  */
 function node_delete_confirm($form, &$form_state, $node) {

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:

 /**
- * Process variables for node.tpl.php
+ * Processes variables for node.tpl.php.
  *
  * Most themes utilize their own copy of node.tpl.php. The default is located
  * inside "modules/node/node.tpl.php". Look in there for the full list of
  * variables.
  *
- * The $variables array contains the following arguments:
- * - $node
- * - $view_mode
- * - $page
+ * @param $variables
+ *   An array containing the following arguments:
+ *   - $node
+ *   - $view_mode
+ *   - $page
  *
  * @see node.tpl.php
  */

$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.)

Albert Volkman’s picture

Status: Needs work » Needs review
FileSize
53.1 KB

Okay, I've now addressed those issues in this patch. I unfortunately cannot create an interdiff as the original patch won't apply cleanly.

jhodgdon’s picture

Status: Needs review » Needs work

Thanks! 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:

+  // Reset the internal static cache of _node_types_build() and forces a
+  // rebuild of the node type information.

forces -> force would be good. :)

2. Regarding (b)... Form constructors shouldn't bother with @return:

+ *
+ * @param $type
+ *   Content type object.
+ *
+ * @return
+ *   Form array for delete confirmation form.
+ *
+ * @see node_type_delete_confirm_submit()
  */
 function node_type_delete_confirm($form, &$form_state, $type) {

The @return should just be removed.

3. And this one isn't quite right: the parameter name is $node not $nodes:

+ * Form constructor for the node add/edit form.
+ *
+ * @param array $nodes
+ *   Array of node nids to update.
+ *
+ * @see node_form_delete_submit()
+ * @see node_form_build_preview()
+ * @see node_form_validate()
+ * @see node_form_submit()
+ * @see node_form_submit_build_node()
+ * @ingroup forms
  */
 function node_form($form, &$form_state, $node) {

4. node_delete_confirm() has the same problem as (3).

5. This one still needs an @param:

+ * Form constructor for the reversion confirmation form.
+ *
+ * This form prevents against CSRF attacks.
+ *
+ * @see node_menu()
+ * @see node_revision_revert_confirm_submit()
  */
 function node_revision_revert_confirm($form, $form_state, $node_revision) {

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:

+ *   An associative array with the following arguments:

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.

Albert Volkman’s picture

1-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.

Albert Volkman’s picture

Status: Needs work » Needs review
jhodgdon’s picture

Regarding (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.

jhodgdon’s picture

Version: 7.x-dev » 8.x-dev
Status: Needs review » Needs work

Let'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.

jhodgdon’s picture

Issue tags: +Novice

tag

Lars Toomre’s picture

I 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.

Lars Toomre’s picture

Status: Needs work » Needs review
FileSize
22.36 KB

Here 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.

Lars Toomre’s picture

Oh, I meant to say, in a couple of places, I used '???' wher I was uncertain. Suggested corrections are welcome.

jhodgdon’s picture

Status: Needs review » Needs work

Thanks 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:

+ *   A page array suitable for use by drupal_render().
+ *
+ *
  * @see node_menu()
  */
 function node_page_view(Node $node) {

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!

Lars Toomre’s picture

Thanks @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.

Lars Toomre’s picture

Status: Needs work » Needs review
FileSize
40.13 KB

As 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?

Lars Toomre’s picture

FileSize
22.74 KB

Let's remember to rebase before rolling the patch. Here is a smaller patch.

jhodgdon’s picture

RE #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:

+ *
+ * @return
+ *   Returns NULL if the user can bypass node access, or if there are no node
+ *   access modules, or if the $account has a global view grant in a view
+ *   operation; otherwise, does not return a value.

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.

Lars Toomre’s picture

Attached 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.

Lars Toomre’s picture

@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?

jhodgdon’s picture

RE #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.

jhodgdon’s picture

Assigned: Albert Volkman » Unassigned
Status: Needs review » Active

Looks good! I committed the patch in #78.

Can we have one more follow-up: in node.module that I noticed while reviewing this patch:

/**
  * Post-processing for node_access_rebuild_batch.

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.

Lars Toomre’s picture

Assigned: Unassigned » Lars Toomre

I 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.

Lars Toomre’s picture

I 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.

Lars Toomre’s picture

Assigned: Lars Toomre » Unassigned
Status: Active » Needs review
FileSize
97.58 KB

This 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.

Status: Needs review » Needs work

The last submitted patch, 1313980-84-node-docs.patch, failed testing.

Lars Toomre’s picture

Status: Needs work » Needs review
FileSize
97.54 KB

Fixes a stray character at top of SummaryLengthTest.php file. Let's see what further the bot thinks.

Lars Toomre’s picture

I 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.

jhodgdon’s picture

Status: Needs review » Needs work

Thanks 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)

+++ b/core/includes/common.inc
@@ -1,17 +1,20 @@
 <?php
 
+/**
+ * @file
+ * Common functions that many Drupal modules will need to reference.
+ */
+
 
...
 /**
- * @file
- * Common functions that many Drupal modules will need to reference.
- *
  * The functions that are critical and need to be available even when serving
  * a cached page are instead located in bootstrap.inc.
  */

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)

- * @see node_type_form_validate()
  * @see node_type_form_submit()
+ * @see node_type_form_validate()
+ *
  * @ingroup forms
  */
 function node_type_form($form, &$form_state, $type = NULL) {

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:

- * @see hook_node_predelete()
  * @see hook_node_delete()
+ * @see hook_node_predelete()

d)

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeAccessRecordsTest.php
@@ -8,7 +8,7 @@
 namespace Drupal\node\Tests;
 
 /**
- * Test case to verify hook_node_access_records functionality.
+ * Test case to verify hook_node_access_records() functionality.
  */
 class NodeAccessRecordsTest extends NodeTestBase {

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:

+   * Checks if custom node ids are saved properly during an import operation.

- Grammar: "Checks *whether* " would be better. [I see this a lot in this patch.]
- Spelling: ids -> IDs

g)

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeTypePersistenceTest.php
@@ -20,7 +20,7 @@ public static function getInfo() {
   }
 
   /**
-   * Test node type customizations persist through disable and uninstall.
+   * Tests node type customizations persist through disable and uninstall.

Needs "that" after Tests.

h)

/**
  * Form submission handler for node_filter_form().
- *
- * @see node_admin_content()
- * @see node_admin_nodes()
- * @see node_admin_nodes_submit()
- * @see node_admin_nodes_validate()
- * @see node_filter_form()
- * @see node_multiple_delete_confirm()
- * @see node_multiple_delete_confirm_submit()
  */
 function node_filter_form_submit($form, &$form_state) {

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:

/**
- * Rename node.language field to node.langcode.
+ * Rename name of node.language field to node.langcode.
  */
 function node_update_8002() {

This makes it redundant -- rename name ?!?

k)

/**
- * Processes variables for node.tpl.php.
+ * Returns HTML for node.tpl.php.
*
  * Most themes utilize their own copy of node.tpl.php. The default is located
  * inside "modules/node/node.tpl.php". Look in there for the full list of
  * variables.
  *
  * @param $variables
- *   An array containing the following arguments:
- *   - $node
- *   - $view_mode
- *   - $page
+ *   An associative array containing:
+ *   - elements: An array of elements to display in view mode.
+ *   - node: A node object.
+ *   - view_mode: View mode, e.g., 'full', 'teaser'...

(this is for node_preprocess_node() function)

- 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):

- *   The operation to be performed on the node. Possible values are:
- *   - "view"
- *   - "update"
- *   - "delete"
- *   - "create"
+ *   The operation to be performed on the node. Possible values are: 'view',
+ *   'update', 'delete', and 'create'.

m) The * got lost here:

- * 'node_access'; when this function returns TRUE, no node access joins are
+  'node_access'; when this function returns TRUE, no node access joins are

n) Needs to start with a verb:

/**
- * Helper for node access functions.
+ * Helper function for node_access functions.

and why did you make this node_access? It was fine as it was (prose, not referring to a particular function).

o)

+++ b/core/modules/node/tests/modules/node_access_test/node_access_test.module
@@ -2,13 +2,14 @@
 
 /**
  * @file
- * Dummy module implementing node access related hooks to test API interaction
+ * A dummy module implementing node access related hooks for testing purposes.
+ *
+ * A summy module implementing node access related hooks to test API interaction

typo on that last line. :)

Lars Toomre’s picture

Thanks 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.

jhodgdon’s picture

That is your choice Lars. I am quite happy to review/commit these API docs cleanup patches.

Albert Volkman’s picture

Status: Needs work » Needs review
FileSize
27.47 KB
80.97 KB

Here's my attempt to resolve the issues presented by @jhodgdon. I cleaned reworked @Lars Toomre patch to apply cleanly to create an interdiff.

jhodgdon’s picture

Status: Needs review » Needs work

Eek! 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)

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeCreationTest.php
/**
-   * Create a "Basic page" node and verify its consistency in the database.
+   * Creates a "Basic page" node and verify its consistency in the database.
    */
   function testNodeCreation() {
...
/**
-   * Create a page node and verify that a transaction rolls back the failed creation
+   * Verifies that a transaction rolls back the failed creation
    */
   function testFailedPageCreation() {

- verify -> verifies in first block
- second one is missing . at end

b)

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeQueryAlterTest.php
+   * node_query_node_access_alter().  We do this by checking that a user which
+   * normally would not have view privileges is able to view the nodes when we
+   * add a record to {node_access} paired with a corresponding privilege in
+   * hook_node_grants().
    */
   function testNodeQueryAlterOverride() {

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

- * @see hook_node_grants()
  * @see hook_node_access_records()
  * @see hook_node_access_records_alter()
+ * @see hook_node_grants()
+ *
  * @ingroup node_access
  */
 function hook_node_grants_alter(&$grants, $account, $op) {

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)

* @param string $op
- *   The operation to be performed. Possible values:
- *   - "create"
- *   - "delete"
- *   - "update"
- *   - "view"
+ *   The operation to be performed. Possible values: 'create', 'delete',
+ *   'update', or 'view'.

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():

/**
- * Helper for node access functions.
+ * Prepares node access functions.

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():

* @param Drupal\node\Node $node
  *   The $node being written to. All that is necessary is that it contains a
- *   nid.
+ *   nid member.

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

+ *
+ * @ingroup batch
  */
 function node_access_rebuild($batch_mode = FALSE) {

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

Albert Volkman’s picture

Status: Needs work » Needs review
FileSize
9.82 KB
76.79 KB

Here ya go.

jhodgdon’s picture

Status: Needs review » Needs work

A 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.

Albert Volkman’s picture

Status: Needs work » Needs review
FileSize
7.02 KB
70.34 KB

@ingroup removed from node_mass_update()

Non-node module updates removed (looks like they were introduced back in #84

Status: Needs review » Needs work
Issue tags: -Novice, -Needs backport to D7, -docs-cleanup-2011

The last submitted patch, node_docs-1313980-95.patch, failed testing.

jhodgdon’s picture

Status: Needs work » Needs review

#95: node_docs-1313980-95.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Novice, +Needs backport to D7, +docs-cleanup-2011

The last submitted patch, node_docs-1313980-95.patch, failed testing.

jhodgdon’s picture

Uh oh. The first time, a couple of unrelated tests failed, so I clicked "re-test". Now it looks like it needs a re-roll.

Albert Volkman’s picture

Status: Needs work » Needs review
FileSize
70.19 KB

Re-rolled

jhodgdon’s picture

Status: Needs review » Needs work

Thanks! 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:

+ *   - base: (required) The base string used to construct callbacks
+ *     corresponding to this node type (i.e. if base is defined as example_foo,
+ *     then example_foo_insert will be called when inserting a node of that
+ *     type). This string is usually the name of the module, but not always.

and a little bit down, a double ..:

+ *   - help: (optional) Help information shown to the user when creating a node
+ *     of this type.. Defaults to an empty string.

b) And this kind of change is really totally unnecessary and just adds clutter:

- *   Not used for this action.
+ *   Not used for this action. Defaults to an empty array.

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).

Albert Volkman’s picture

Status: Needs work » Needs review
FileSize
8.52 KB
67.1 KB

I completed the items specifically noted by @jhodgdon, but didn't go any further.

jhodgdon’s picture

Status: Needs review » Needs work

Those 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:

 /**
- * Inform the node access system what permissions the user has.
+ * Informs the node access system what permissions the user has.
  *
  * This hook is for implementation by node access modules. In this hook,

Quick reroll for that and we can call this one RTBC. Thanks!

Albert Volkman’s picture

Status: Needs work » Needs review
FileSize
492 bytes
66.8 KB

Done!

jhodgdon’s picture

Assigned: Unassigned » jhodgdon
Status: Needs review » Reviewed & tested by the community

Great! 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.

jhodgdon’s picture

Version: 8.x-dev » 7.x-dev
Assigned: jhodgdon » Unassigned
Status: Reviewed & tested by the community » Patch (to be ported)

Gracious! 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. :)

nod_’s picture

Issue tags: +JavaScript

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.

jhodgdon’s picture

RE #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?

nod_’s picture

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.

jhodgdon’s picture

In 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!

Albert Volkman’s picture

Status: Patch (to be ported) » Needs review
FileSize
56.92 KB

First pass at this.

jhodgdon’s picture

Status: Needs review » Needs work

Thanks 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:

+++ b/modules/node/node.admin.inc
@@ -7,6 +7,7 @@
 
 /**
  * Menu callback: confirm rebuilding of permissions.
+ * @ingroup forms
  */
 function node_configure_rebuild_confirm() {

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!

Albert Volkman’s picture

Status: Needs work » Needs review
FileSize
19.31 KB
72.93 KB
890 bytes
56.36 KB

So 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).

jhodgdon’s picture

Regarding 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!

Albert Volkman’s picture

Issue tags: -Novice

I actually copied most of them from D8.

jhodgdon’s picture

Oh cool. I'll give it a review then and get it committed, probably Monday. Thanks!

Albert Volkman’s picture

Backpedaling 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)?

jhodgdon’s picture

RE #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!

Albert Volkman’s picture

Version: 7.x-dev » 8.x-dev
FileSize
512 bytes

Here's a patch for #117.

jhodgdon’s picture

Assigned: Unassigned » jhodgdon
Status: Needs review » Reviewed & tested by the community

Thanks! I'll get that one committed, and then go back and review (and hopefully commit) the 7.x patch in #113.

jhodgdon’s picture

Version: 8.x-dev » 7.x-dev
Assigned: jhodgdon » Unassigned
Status: Reviewed & tested by the community » Needs review

Patch 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!

jhodgdon’s picture

Status: Needs review » Needs work

I 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

+ * - Loading a node (calling node_load(), node_load_multiple(), entity_load(),
  *   entity_load() with $entity_type of 'node'):

entity_load() is duplicated here.

b) node.module

- *   An array of partial node objects or an empty array if there are no recent
- *   nodes visible to the current user.
+ *   !n array of node entities or an empty array if there are no recent nodes
+ *   visible to the current user.
  */
 function node_get_recent($number = 10) {

Typo in the first line !n instead of An

c) node.pages.inc

+/**
+ * Validates settings form for node_form().
+ *
+ * @see node_form()
+ */
 function node_form_validate($form, &$form_state) {

Shouldn't this be documented as a standard form validation function?

d) node.pages.inc

+/**
+ * Saves settings form for node_form().
+ *
+ * @see node_form()
+ * @see node_form_validate()
+ */
 function node_form_submit($form, &$form_state) {

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!!

Albert Volkman’s picture

Status: Needs work » Needs review
FileSize
4.46 KB
74.27 KB

Here ya go. Found a couple more too. Interdiff'd against 113b.

jhodgdon’s picture

Assigned: Unassigned » jhodgdon
Status: Needs review » Reviewed & tested by the community

Excellent! I'll get this committed when it turns green. :)

jhodgdon’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 7.x, and there was much rejoicing!!!!!

Automatically closed -- issue fixed for 2 weeks with no activity.

jenlampton’s picture

Issue summary: View changes
Status: Closed (fixed) » Needs work

One 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.

jenlampton’s picture

Status: Needs work » Closed (fixed)

Whoops, maybe not. Looks like that was fixed over in https://drupal.org/node/2117601