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 |
---|---|---|---|
#31 | path_docs_cleanup-1355682-31.patch | 9.06 KB | Zgear |
#25 | path_docs_cleanup-1355682-24.patch | 9.16 KB | Zgear |
#21 | path_docs_cleanup-1355682-21.patch | 9.4 KB | Zgear |
#18 | path_docs_cleanup-1355682-18.patch | 9.4 KB | chertzog |
#14 | path_api_docs-1355682-14.patch | 8.65 KB | chertzog |
Comments
Comment #1
Sree CreditAttribution: Sree commentedupdating the title ...
Comment #2
jhodgdon@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.
Comment #3
chertzogHere 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...
Comment #4
jhodgdonThanks 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:
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:
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):
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. :)
Comment #5
chertzogSorry, I thought that the clean up was just for the *.api.php files. I will work on a patch for the entire module.
Comment #6
chertzogThink i got everything.
Comment #7
jhodgdonLooks pretty good! A few things need to be fixed:
a) path.admin.inc
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
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
I don't think Path needs to be capitalized here. Or here for that matter:
Comment #8
chertzogUpdated.
Comment #9
xjmThanks @chertzog. I found a few things (just read the patch; didn't apply it):
Typo ("contructor").
The submission and validation handlers don't need
@see
to the main constructor, because it is already linked in the summary.Are these button submission handlers? If so we should update the summaries to the standard.
Comment #10
chertzogHere 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.Comment #11
jhodgdonWe are normally adding @see between all the validate/submit handlers for a given form, so yes I think they would be good. Thanks!
Comment #12
chertzogPatch with @see for the filter and reset buttons.
Comment #13
xjmSpotted a couple more typos:
Typo here (comma instead of period).
Typo: "contructor."
I also applied the patch and reviewed with it applied:
testTermAlias()
method inpath.test
needs its verb form fixed still.path_admin_edit()
needs its summary tweaked to the new menu callback standard.Comment #14
chertzogFixed, and updated.
Comment #16
chertzog#14: path_api_docs-1355682-14.patch queued for re-testing.
Comment #17
jhodgdonLooks better! Unfortunately, I noticed a few more things that need fixing:
a)
This description... "handles pages" ?!? This function actually returns a form for creating or editing a URL alias. Description needs a rewrite.
b)
This needs documentation of the $path argument added.
c)
path_admin_filter_form_submit() actually doesn't exist.
d) As long as we're making a new patch anyway...
Normally we document hooks like "Respond to a path..." rather than "Allow modules to respond to a path...".
e)
The form_alter function should probably have an @see to the validate function?
f)
In last line, id -> ID.
Comment #18
chertzogNew patch attached.
Comment #19
xjmHey @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
Comment #20
jhodgdonGreat 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.
Comment #21
Zgear CreditAttribution: Zgear commentedthis how to reroll properly?
Comment #22
xjmComment #24
jhodgdonZgear: it looks like an 8.x patch. This is now a 7.x issue.
Comment #25
Zgear CreditAttribution: Zgear commentedmy 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?
Comment #26
Zgear CreditAttribution: Zgear commentedComment #28
Zgear CreditAttribution: Zgear commented#25: path_docs_cleanup-1355682-24.patch queued for re-testing.
Comment #30
Zgear CreditAttribution: Zgear commentedsure enough I did something wrong, hold on a bit longer as I straighten this out.
Comment #31
Zgear CreditAttribution: Zgear commentedheh just a simple mistake with sublime text, anyways this should pass through. Sorry for the test spam.
Comment #32
Zgear CreditAttribution: Zgear commentedComment #33
jhodgdonThird 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.
Comment #34
xjmThat backport looks good. Thanks @Zgear!
Comment #35
jhodgdonCommitted to 7.x, thanks all!
Comment #37
Lars Toomre CreditAttribution: Lars Toomre commentedFor reference, I created a follow-up issue #1811948: Further clean up of API docs for Path module.