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 |
---|---|---|---|
#76 | filter_docs-1347914-76.patch | 38.16 KB | Albert Volkman |
#73 | filter_docs-1347914-73.patch | 18.5 KB | Albert Volkman |
#73 | interdiff.txt | 829 bytes | Albert Volkman |
#71 | filter_docs-1347914-71.patch | 18.5 KB | Albert Volkman |
#71 | interdiff.txt | 4.69 KB | Albert Volkman |
Comments
Comment #1
jhodgdon@sven.lauer: Do you still plan to work on this? If so, please respond. If not, we'll unassing so someone else can. Thanks!
Also, tagging.
Comment #2
sven.lauer CreditAttribution: sven.lauer commentedDuh, I kinda half-forgot that I was working on this. I'll post a first patch soon (or at least my partial patch and unassign). Sorry about that.
Comment #3
sven.lauer CreditAttribution: sven.lauer commentedOkay, it seems like I don't have the time to do this soon, so unassigning.
I attach a partial patch. From looking at it, I think it is only (part of) filter.module that needs to be done, but no guarantees.
(The patch applies against the current 8.x branch.)
Comment #4
xjmMarking NW since there's a partial patch. Thanks @sven.lauer!
Comment #5
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedHopefully I didn't miss too much on this one but it is a big project compared to what I have been working on.
Comment #7
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedComment #8
jhodgdon#7: doc-cleanup-filter-module-1347914-7.patch queued for re-testing.
Comment #9
jhodgdonSorry this got stuck at "needs review". Let's see if the test bot is still happy with it, then get it reviewed and committed!
Comment #11
jhodgdon#7: doc-cleanup-filter-module-1347914-7.patch queued for re-testing.
Comment #13
jhodgdon#7: doc-cleanup-filter-module-1347914-7.patch queued for re-testing.
Comment #14
xjmGot halfway through my review and hit the wrong button in dreditor, but here's what I had up to that point:
This blank line is there in all core files, as far as I know, so let's not remove it? (And add it in the other files.)
How about:
Form constructor for a form to list and reorder text formats.
Period needed here, I think.
See http://drupal.org/node/1354#render for how to document this guy.
This parameter is optional, correct?
These need some work. In particular I think "Filters the tips callback" is incorrect. They're (apparently) callbacks for filter tips, not things filtering "tips callbacks."
I think there needs to be a blank line here as well.
"A given other string" seems awkward (or maybe it's just me?) Maybe "another provided string"?
Hmm, I think this is a bit awkward. Maybe lose this paragraph and make the summary say something like:
Asserts that a lowercased, decoded HTML string contains another.
(or something like that?) This actually would fix the previous point (and it applies to the next change in the patch as well).
Comment #15
xjmOK I guess I was almost through the patch. One other thing:
Maybe just "etc." rather than "and similar things"? :)
I didn't apply the patch locally to check for missed bits; the above points are just from reading the patch.
Comment #16
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedxjm,
For #4 where the documentation says "- Somewhere in the function, there should be a paragraph saying where the callback is assigned and what particular Render API callback it is being used for."
Since the function itself never gives any other details should I add that it is a #pre_render callback in the long description?
For the first comment in #6 would this be appropriate? Also I'm not sure what to put for the within. It is called from filter_filter_info() and hook_filter_info() but neither of those really seem to follow the example of uasort() within system_themes_page().
* Provides tips based on the HTML filter.
*
* Callback for _filter_tips() within system_themes_page().
Comment #17
xjmThis seems correct to me. I'd also suggest grepping core for how this callback is used (grep the function name without parens) and see if that is worth documenting.
Checking on the other thing now.
Comment #18
xjmSo whaddya know, there is such a thing as a "tips callback." (This is one of the defined keys for
hook_filter_info()
, in a similar vein tohook_menu()
.) It's not really "based on" the HTML filter though--it's intended for it. Maybe something along the lines of:You'd want to look each one up to check the specifics.
Comment #19
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedBelow is how I modified the following functions with the new way below each. There are two of them that exceed 80 characters but I'm not sure what to do in this case because the callback takes up almost 40 of the 80 characters.
Additonally since there are three other callbacks to preg_replace should I update those as well such as.
Comment #20
jhodgdonHmmm.
This could be made a few characters shorter as:
That would at least give us a few extra characters to work with. I think we are stuck with the name "Filter tips callback", because "tips callback" is what it is called in hook_filter_info(). These should probably have @see to the relevant hook_filter_info() implementation too? Oh, you already have that. Good.
On the other set:
In this case, it is a callback for preg_replace(). We have a standard for this type of thing:
http://drupal.org/node/1354#callbacks
So this should be:
Comment #21
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedjhodgdon,
Thanks for the quick reply. The change you mentioned got all of comments under the 80 character limit.
Hopefully this patch corrects all of the issues identified so far.
Comment #22
jhodgdonThanks for the new patch!
I took a look at the current patch, all the way through. A few things that should be fixed:
a) In filter.admin.inc, there are a few form constructors that are missing @param docs (only needed for the parameters other than $form, $form_state).
b) In filter.admin.inc, there are a couple of page callbacks that are missing @param/@return. These are required under current standards:
http://drupal.org/node/1354#menu-callback
c) In filter.api.php -- for hook docs, the verb tense is different, so these changes need to be put back to what they are:
http://drupal.org/node/1354#hooks
d) In filter.module:
Normally we don't use @code to show structure of returned arrays. I would rewrite this @return section to say:
An associative array of filtering tips, keyed by filter name. Each filtering tip is an associative array with elements:
- tip: Tip text.
- id: Filter ID.
e) filter.module
Settings should not be capitalized.
f) filter.module
Needs newline after the initial doc line.
g) filter.module
an -> the
That's it! The rest of the patch looks great. Thanks!
Comment #23
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedI took care of most of your comments.
For b) there were two that I did not include the return values for. filter_admin_format_form() where it returns the form and filter_admin_format_page() where it returns drupal_get_form('filter_admin_format_form', $format); I grepped core and couldn't find a single place these were documented with the return values.
For g) I changed it but that puts it at 81 characters.
Comment #24
jhodgdong) Actually, these callbacks should uniformly be called: "Filter tips callback: Specific description here." Normally these one-liners with : have a standard part before the colon, and the specific part after.
Comment #25
jhodgdonOh, and on (b) - If it's a page callback, it should have a return value stating either that it returns a renderable array or a form array or a string or whatever. If it's a form constructor, it should be documented with the format of a form constructor (no return value necessary). So this one:
still needs a return value, unless it should say "Form constructor for the...".
Comment #26
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedSorry for the delay.
I tried looking for an example of how to give a return value for
but I can't find an example in the documentation that has already been fixed documenting a return value for drupal_get_form. I don't want to sound like I'm disagreeing with you but I don't know what the proper return value is here unless you want me to say "Returns a renderable form array for the filter_admin_format_form()." which doesn't seem right to me.
Comment #27
jhodgdon@return
A form array.
would be enough, given that the one-line description says which form array it is. That is consistent with other page callback return statements we have been putting in core.
Comment #28
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedHere is the latest.
Comment #30
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedComment #32
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedApparently the filter.css file was deleted. Hopefully this patch will apply.
Comment #33
xjmThanks @NROTC_Webmaster. I reviewed the full patch again and found just a few things:
Typo: "hanlder".
"Boolean" should be capitalized ("A Boolean indicating..."). (It's derived from a name.)
I think "markup" should be one word (no hyphen).
I wonder if these two sentences could be merged into one paragraph?
Typo ("whehter").
I'll also apply the patch locally and see if anything was missed. An interdiff showing the changed lines from the previous patch would again be helpful.
Comment #34
xjmI found a few other things to fix when I reviewed the module with the patch applied:
FilterAdminTestCase
,FilterFormatAccessTestCase
,FilterDefaultFormatTestCase
, andFilterNoFormatTestCase
are still missing docblocks._filter_html_settings()
,_filter_html()
,_filter_url()
,Drupal.behaviors.filterGuidelines
, and several callback hooks infilter.api.php
have nonstandard docblocks yet.Comment #35
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedI think I took care of all of the issues except the callbacks. I have a hard time understanding how they should be documented.
For _filter_html_settings should it be
Additionally, when the docblock has @code is the code allowed to go past the 80 char limit? It doesn't describe it in the documentation so I didn't change it.
Comment #36
jhodgdon@code can go past 80 characters I guess, but it would probably be nicer to break it up.
Regarding that filter settings callback... we haven't really made standards for these, but since there is "hook" documentation for hook_filter_FILTER_settings(), I think we could do:
Notes:
- That second line might need to be wrapped to fit 80 characters
- I took the term "content filter" from hook_filter_info().
Comment #37
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedUpdated per #36.
Interdiff from patch in 32.
Comment #38
jhodgdonIt's looking pretty good! A few problems:
a) filter.admin.inc
That @return is not accurate. This is a theme function and it returns HTML. We don't need the @return there actually. See http://drupal.org/node/1354#themeable
b) As long as we're fixing that (filter.admin.inc again):
Could use a "the" in there.
c) And in quite a few places in filter.admin.inc:
This is unclear. If it's an associative array, what are the keys? And what is a "format" -- an array or an object, with what components? Is the name a machine name or a translated human-readable name?
d)
Should be Define here. See http://drupal.org/node/1354#hooks -- and the same applies to the other hook definitions in this file.
e)
Actually, "filter" should stay lower-case here, as I think it's referring to a module that provides a filter to Drupal, not to the core Filter module.
f) filter.module
How about "having" the properties in the last line here? There's another "using the properties" below in the same function header doc too. It doesn't make sense to me, because we're documenting what parameters to pass into the function here. Here's the other spot:
g) filter.module
This is not a form title. It's a page title, or really just the name of a text format being used as a page title. But the function is also called directly from filter_format_fallback_title() apparently (though maybe that function is never used?) so maybe we shouldn't even be documenting this as a title callback?
Comment #39
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedIn regards to c) it is documented that way 8 times in 3 files.
I guess what I'm asking is what would you like it to say.
* An associative array containing:
* - format: A string used as the primary key to describe the new format.
* - name: A string to label the new format.
I could also give an example like
-format: filtered_html
-name: Filtered HTML
The format and name are simply varchar(255) with format being the primary key. They reference what filters are applied to what roles. (Unless I missed something)
Comment #40
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedActually, since I'm about to leave I'm going to just go ahead and make the change as described. Please let me know if you think it should be something else.
Comment #41
jhodgdonOK, what's the array key? Ooooooh, I finally think I see what this actually is... If I have understood properly, then the docs should say something like this:
A one-element associative array whose key is the machine name for the format, and whose value is the human-readable name of the format.
Is that right? It sounds like that is what you are saying this contains? If it's really a multi-element array, then it should say something like:
An associative array of formats, keyed by machine name. Each value is an array with elements:
- format: The machine name of the format.
- name: The human-readable name assigned to the format.
But I haven't looked at the code to see what this array really is... please verify!
Comment #42
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedActually, I was wrong before. After going through this format is an object. Please review the latest patch to make sure it is correct. The first two instances are the same with the third being simplified because of the function.
interdiff from #37
Comment #43
xjm@NROTC_Webmaster, can you create your interdiffs using git or with patchutils? The diff-of-diffs is hard to read. Thanks!
Comment #44
xjmThe patch looks good. I didn't yet apply it locally or look up the individual functions for the new parameter and return documentation. I only found one issue in the patch itself, plus a question:
I think this parameter is optional, so we should document it as such and probably explain the fallback behavior.
I'm now thinking we should maybe document these like the Render API callbacks... thoughts?
Comment #45
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedI will definitely add the optional to the first item.
Regarding the second I don't have a preference either way. I personally just like to follow a standard that I can apply in every case to simplify my life and make things orderly. If we are going to go with the Render API callback form I would like to see it explained a little more clearly in the documentation. I personally find it a little vague. The main problem being that I'm not quite sure when to use it and when not to. Let me look through it and I will see if I can come up with something that makes sense to me and passes your standard as well.
Comment #46
jhodgdonRegarding the question in #44, these filter callbacks are pretty much only used in Filter module, so I think it's OK to do them as they are in this patch... but what would they look like if they were "like Render API callbacks"?
Comment #47
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedActually I have a question for you.
What do you think is the best way to list this is as optional? I don't like what I have but I don't know of a better way.
Comment #48
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedMy assumption was that they would have
instead of just the @see
Comment #49
jhodgdonUm... for one thing, in text use NULL not null. But actually, I wouldn't say "or can be NULL" anyway. You already have object|null as the type, and (optional) there, and presumably it's $format = NULL in the function signature, so that clause doesn't add anything really?
I would just say:
(optional) An object representing a format, with the following properties:
Comment #50
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedHere is an update and iterdiff from #42 with the exception of changing the callbacks.
Comment #51
jhodgdonLooks mostly good! I gave this patch a complete review (sorry for the delay!) and found the following problems:
a) in filter.admin.inc:
The return value here is a string, not a renderable array (this is a theme function!). But our standards say to omit the @return for theme functions anyway: http://drupal.org/node/1354#themeable
b) filter.admin.inc:
This is not accurate. This form is for *disabling*, not deleting, a format. The callback called _filter_disable_format_access() in filter.module has the same problem.
c) in filter.module - filter_process_format() docs:
id -> ID
d) in filter.module:
This type of function header should have been left alone, see http://drupal.org/node/1354#themeable -- several (maybe just two?) were changed like this:
e)
The added line should come before the @param section.
f) filter.test:
typo: adminstration -> administration
Comment #52
batigolixgonna give this a try
Comment #53
batigolixThe attached patch incorporates comments from #51.
Regarding comment #51 f: the file filter.test seems to be removed.
I added comments for functions in the core/modules/filter/lib/Drupal/filter/Tests that weren't documented yet.
Comment #55
jhodgdonRegarding the .test file, see http://drupal.org/node/1418980#comment-6148434
Comment #56
batigolixthanks for the link. I *did* search through all files for the "adminstration" typo. The fixes are included in the patch at #54
Comment #57
batigolixchanging the status
Comment #58
jhodgdon#53: doc-cleanup-filter-module-1347914-53.patch queued for re-testing.
Comment #59
jhodgdonThis looks pretty good, thanks -- and sorry no one reviewed it until now... I did find a few problems:
a)
This is not accurate. The return value is an HTML string (that is what theme functions almost always return). Actually, you don't need a @return here anyway, see:
http://drupal.org/node/1354#themeable
b)
This is the submission handler for filter_admin_disable() actually.
That's it -- the rest of the patch looks good! If we can get these two items changed, that would be great.
Comment #60
batigolixallright, here's the new patch
Comment #61
batigolixset the status to review
Comment #62
jhodgdonWhen you make a small change on a large patch, it's very helpful for reviewers if you can include an interdiff, so that they don't have to review the whole patch. Instructions:
http://drupal.org/node/1488712
Comment #63
jhodgdonThis is looking really good! I went ahead and committed the patch, because it is 99.9% great. We need a quick follow-up to fix these three items:
a)
This line is longer than 80 characters.
b)
This needs to have a one-line 80-character summary.
c) same as (a):
Comment #64
batigolixattached patch incorporates comments from #63
Comment #65
Cameron Tod CreditAttribution: Cameron Tod commentedHi batgolix, thanks for your help on this. It seems like your patch changes different docs than those mentioned in #63, could you double check that you're changing the right comments?
a)
Out of scope for this issue.
b)
There's a small trailing whitespace issue here.
c)
This is a little unclear, how about something like
"Tests removal of filtered content when an active filter is disabled."
Comment #66
batigolixoops, that was a left over from another issue I ve been working on this morning.
attached a new patch that addresses the comments from #65
Comment #67
batigolixset status
Comment #68
jhodgdonThanks! Sorry again for the delay on committing these patches... this one is committed. I think it's time to port both this latest patch and the one in #60 (combined, that is) to 7.x.
Comment #69
Lars Toomre CreditAttribution: Lars Toomre commentedI spent time this evening reading through the entire Filter module and saw that there still a number of issues with the tests in this module. Attached is a locally untested patch that documents what I found.
The filter callback functions in the middle of the filter.module file deserve another look. The documentation for those are inconsistent still and it is unclear to me which ones, if at all, need @param and @return directives.
I also opened issue #1811638: [meta] Clean-up Test members - ensure property definition and use of camelCase naming convention to address a separate coding convention issue that I noticed in the tests.
Comment #70
jhodgdonThanks! This all looks good except:
a)
Some information was lost here -- it should probably say "Defaults to NULL, indicating neither." on that 2nd line.
b)
These appear to be stray lines added to FilterAdminTest.php that don't document anything?
c)
I don't think "filter" here is referring to the "Filter module", but just "filters", so the previous lower-case text was actually correct. Also applies to some of the other classes.
d) Changes like this:
I'm not aware that we have a standard saying that there should be a blank line at the end of a class just before the closing } or at the top. In any case, it's not a documentation standard, and these types of changes do not belong in this patch.
Comment #71
Albert Volkman CreditAttribution: Albert Volkman commentedI had to re-roll the initial patch to get an interdiff, and I accidentally left some of the extra newline removals out of the re-roll. Therefore the interdiff isn't complete, but the new patch is free of newlines.
Comment #72
jhodgdonThere's a small indentation mishap here in filter.module, which I probably missed in the last review, sorry:
Everything else looks good, thanks!
Comment #73
Albert Volkman CreditAttribution: Albert Volkman commentedHere we go.
Comment #74
jhodgdonThanks! I'll get this committed soon.
Comment #75
jhodgdonCommitted this patch!
I think it's now time to go back to 7.x and port a combination of this patch in #73 and the one in #66 and the one in #60, as much as possible anyway. Thanks!
Comment #76
Albert Volkman CreditAttribution: Albert Volkman commentedBackported.
Comment #77
jhodgdonExcellent work, as usual! This is ready for commit (can't do it right now, running out the door...). Thanks! I see a few places where we could go back to D8 and fix things again, but nothing major, so I think the best thing is just to get this FINISHED. :)
Comment #78
jhodgdonCommitted to 7.x, with the exception of the JS files (I've been asked not to add file doc headers to JS files for Drupal 7, as we are not minifying JS in D7 and it adds to overhead).
Comment #79
jhodgdon