Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Sree’s picture

Title: Path Module » Clean up API docs for path module

updating the title ...

jhodgdon’s picture

Issue tags: +Novice, +Needs backport to D7

@surendramohan: Do you still plan to work on this? If so, please respond. If not, we'll unassign it so someone else can. Thanks!

Also, tagging.

chertzog’s picture

Assigned: surendramohan » chertzog
Status: Active » Needs review
FileSize
1023 bytes

Here is a patch that addresses the very few issues. I know i didnt claim it, but seeing as it hasnt had any movement in months...

jhodgdon’s picture

Status: Needs review » Needs work

Thanks for claiming this issue, since the other person didn't respond!

So... Adding param/return data types is actually *not* part of the Cleanup the API Docs plan (see meta-issue for description of what is included). But these three are straightforward enough, so might as well include them.

Looking through the rest of the files in modules/path, I see a few other things that should be fixed up:

a) path.module line 97:

/**
 * Implements hook_form_BASE_FORM_ID_alter().
 */
function path_form_node_form_alter(&$form, $form_state) {

See http://drupal.org/node/1354#hookimpl (should say which form). Same for path_form_taxonomy_form_term_alter around line 230.

b) path.module line 150:

/**
 * Form element validation handler for URL alias form element.
 */
function path_form_element_validate($element, &$form_state, $complete_form) {

Should have @see to the form generate/submit functions, see http://drupal.org/node/1354#forms

c) path.js - should have an @file docblock.

c) path.admin.inc - I noticed immediately that the verb tense was wrong for the first function in there (see http://drupal.org/node/1354#functions):

/**
 * Return a listing of all defined URL aliases.

I stopped looking at this point -- there are likely other issues in that file.

d) path.test -- the class(es) in there are not properly documented -- classes and methods all should have doc blocks (with the exception of the setUp() and getInfo(). And check verb tenses please. :)

chertzog’s picture

Sorry, I thought that the clean up was just for the *.api.php files. I will work on a patch for the entire module.

chertzog’s picture

Status: Needs work » Needs review
FileSize
7.24 KB

Think i got everything.

jhodgdon’s picture

Status: Needs review » Needs work

Looks pretty good! A few things need to be fixed:

a) path.admin.inc

@@ -178,6 +178,8 @@ function path_admin_form($form, &$form_state, $path = array('source' => '', 'ali
 
 /**
  * Submit function for the 'Delete' button on the URL alias editing form.
+ *
+ * @see path_admin_form()
  */
 function path_admin_form_delete_submit($form, &$form_state) {

See http://drupal.org/node/1354#forms -- instead of saying "on the URL ..." it should say "on path_admin_form()" and then you don't need the @see. Also path_admin_form() needs to have an @see to this function, and probably the other submit/validate functions should too (and this should have @see to the other submit/validate functions).

b) path.admin.inc

@@ -229,7 +235,7 @@ function path_admin_form_submit($form, &$form_state) {
 }
 
 /**
- * Menu callback; confirms deleting an URL alias
+ * Menu callback: Confirms deletion of an URL alias.
  */
 function path_admin_delete_confirm($form, &$form_state, $path) {

Needs docs for the param/return... No, actually this is a form generation function, not a menu callback, so it just needs docs to look like form generation and have $path param documented. The submit handler for this (just below) also needs to be documented as a form submission handler.

c) Not to write the same thing over and over again... there are several other places in path.admin.inc and path.module that form submit/validation handlers are not documented according to http://drupal.org/node/1354#forms

d) In path.test... That first class PathTestCase doesn't appear to actually do any testing, so I don't think the one-line description is accurate. It's a base class, so it should say something like "Provides a base class for testing the Path module.".

e) in path.test

@@ -23,6 +26,9 @@ class PathTestCase extends DrupalWebTestCase {
   }
 }
 
+/**
+ * Tests Path alias functionality.
+ */
 class PathAliasTestCase extends PathTestCase {

I don't think Path needs to be capitalized here. Or here for that matter:

@@ -177,6 +183,15 @@ class PathAliasTestCase extends PathTestCase {
     $this->assertResponse(404);
   }
 
+  /**
+   * Returns the Path ID.
chertzog’s picture

Status: Needs work » Needs review
FileSize
7.88 KB

Updated.

xjm’s picture

Status: Needs review » Needs work

Thanks @chertzog. I found a few things (just read the patch; didn't apply it):

  1. +++ b/core/modules/path/path.admin.incundefined
    @@ -229,7 +239,12 @@ function path_admin_form_submit($form, &$form_state) {
    + * Form contructor for the path deletion form.
    

    Typo ("contructor").

  2. +++ b/core/modules/path/path.admin.incundefined
    @@ -245,7 +260,9 @@ function path_admin_delete_confirm($form, &$form_state, $path) {
    + *
    + * @see path_admin_delete_confirm()
    

    The submission and validation handlers don't need @see to the main constructor, because it is already linked in the summary.

  3. +++ b/core/modules/path/path.admin.incundefined
    @@ -291,6 +309,8 @@ function path_admin_filter_form($form, &$form_state, $keys = '') {
      * Process filter form submission when the Filter button is pressed.
    
    @@ -298,6 +318,8 @@ function path_admin_filter_form_submit_filter($form, &$form_state) {
      * Process filter form submission when the Reset button is pressed.
    

    Are these button submission handlers? If so we should update the summaries to the standard.

chertzog’s picture

Status: Needs work » Needs review
FileSize
7.83 KB

Here is a patch. As for the filter and reset buttons, i dont know if they need a @see to each other, as they are not in the same workflow. One does not get called after the other... So i left them out. If they need it I will add them.

jhodgdon’s picture

We are normally adding @see between all the validate/submit handlers for a given form, so yes I think they would be good. Thanks!

chertzog’s picture

FileSize
7.94 KB

Patch with @see for the filter and reset buttons.

xjm’s picture

Status: Needs review » Needs work

Spotted a couple more typos:

+++ b/core/modules/path/path.admin.incundefined
@@ -216,7 +223,10 @@ function path_admin_form_validate($form, &$form_state) {
+ * Form submission handler for path_admin_form(),

Typo here (comma instead of period).

+++ b/core/modules/path/path.admin.incundefined
@@ -229,7 +239,12 @@ function path_admin_form_submit($form, &$form_state) {
+ * Form contructor for the path deletion form.

Typo: "contructor."

I also applied the patch and reviewed with it applied:

  1. The testTermAlias() method in path.test needs its verb form fixed still.
  2. path_admin_edit() needs its summary tweaked to the new menu callback standard.
chertzog’s picture

Status: Needs work » Needs review
FileSize
8.65 KB

Fixed, and updated.

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

The last submitted patch, path_api_docs-1355682-14.patch, failed testing.

chertzog’s picture

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

#14: path_api_docs-1355682-14.patch queued for re-testing.

jhodgdon’s picture

Status: Needs review » Needs work

Looks better! Unfortunately, I noticed a few more things that need fixing:

a)

@@ -88,7 +88,15 @@ function path_admin_overview($keys = NULL) {
 }
 
 /**
- * Menu callback; handles pages for creating and editing URL aliases.
+ * Page callback: Handles pages for creating and editing URL aliases.
+ *
+ * @param $path
+ *   The path id.
+ *
+ * @return
+ *   A form for adding or editing a URL alias.
+ *
+ * @see path_menu()
  */
 function path_admin_edit($path = array()) {

This description... "handles pages" ?!? This function actually returns a form for creating or editing a URL alias. Description needs a rewrite.

b)

@@ -103,11 +111,12 @@ function path_admin_edit($path = array()) {
 }
 
 /**
- * Return a form for editing or creating an individual URL alias.
+ * Form constructor for the path administration form.
  *
  * @ingroup forms
  * @see path_admin_form_validate()
  * @see path_admin_form_submit()
+ * @see path_admin_form_delete_submit()
  */
 function path_admin_form($form, &$form_state, $path = array('source' => '', 'alias' => '', 'langcode' => LANGUAGE_NOT_SPECIFIED, 'pid' => NULL)) {

This needs documentation of the $path argument added.

c)

@@ -255,10 +278,11 @@ function path_admin_delete_confirm_submit($form, &$form_state) {
 }
 
 /**
- * Return a form to filter URL aliases.
+ * Form constructor for the path admin overview filter form.
  *
  * @ingroup forms
- * @see path_admin_filter_form_submit()
+ * @see path_admin_filter_form_submit_filter()
+ * @see path_admin_filter_form_submit_reset()
  */
 function path_admin_filter_form($form, &$form_state, $keys = '') {

path_admin_filter_form_submit() actually doesn't exist.

d) As long as we're making a new patch anyway...

+++ b/core/modules/path/path.api.php
@@ -10,7 +10,6 @@
  * @{
  */
 
-
 /**
  * Allow modules to respond to a path being inserted.
  *

Normally we document hooks like "Respond to a path..." rather than "Allow modules to respond to a path...".

e)

@@ -91,7 +91,7 @@ function path_menu() {
 }
 
 /**
- * Implements hook_form_BASE_FORM_ID_alter().
+ * Implements hook_form_BASE_FORM_ID_alter() for node_form().
  */
 function path_form_node_form_alter(&$form, $form_state) {
   $path = array();
@@ -145,6 +145,8 @@ function path_form_node_form_alter(&$form, $form_state) {
 
 /**
  * Form element validation handler for URL alias form element.
+ *
+ * @see path_form_node_form_alter()
  */
 function path_form_element_validate($element, &$form_state, $complete_form) {

The form_alter function should probably have an @see to the validate function?

f)

@@ -177,6 +183,15 @@ class PathAliasTestCase extends PathTestCase {
     $this->assertResponse(404);
   }
 
+  /**
+   * Returns the path ID.
+   *
+   * @param $alias
+   *   A string containing an aliased path.
+   *
+   * @return int
+   *   Integer representing the path id.
+   */
   function getPID($alias) {

In last line, id -> ID.

chertzog’s picture

Status: Needs work » Needs review
FileSize
9.4 KB

New patch attached.

xjm’s picture

Status: Needs review » Reviewed & tested by the community

Hey @chertzog, thanks for the reroll. I reviewed again and everything in the patch looks good to me!

An aside, one thing you could do in the future is add interdiffs so it is easier to see which changes you've made since previous versions:
http://drupal.org/node/1488712

jhodgdon’s picture

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

Great work! Committed to 8.x.

I think all the changes in here are fine for 7.x, but the patch needs a reroll in order to apply.

Zgear’s picture

this how to reroll properly?

xjm’s picture

Status: Patch (to be ported) » Needs review

Status: Needs review » Needs work

The last submitted patch, path_docs_cleanup-1355682-21.patch, failed testing.

jhodgdon’s picture

Zgear: it looks like an 8.x patch. This is now a 7.x issue.

Zgear’s picture

my bad, this time it should work, however since the class PathAliasTestCase didn't exist in the drupal 7 path.test file I left out the comment "Tests path alias functionality." Is this ok?

Zgear’s picture

Status: Needs work » Needs review

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

The last submitted patch, path_docs_cleanup-1355682-24.patch, failed testing.

Zgear’s picture

Status: Needs work » Needs review

#25: path_docs_cleanup-1355682-24.patch queued for re-testing.

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

The last submitted patch, path_docs_cleanup-1355682-24.patch, failed testing.

Zgear’s picture

sure enough I did something wrong, hold on a bit longer as I straighten this out.

Zgear’s picture

heh just a simple mistake with sublime text, anyways this should pass through. Sorry for the test spam.

Zgear’s picture

Status: Needs work » Needs review
jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Third time's the charm! Looks good Zgear, and thanks for the patch. I'll get it committed soon unless someone else beats met to it.

xjm’s picture

That backport looks good. Thanks @Zgear!

jhodgdon’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 7.x, thanks all!

Status: Fixed » Closed (fixed)

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

Lars Toomre’s picture

For reference, I created a follow-up issue #1811948: Further clean up of API docs for Path module.