Currently in the D8 documentation there are a bunch of functions that take parameters and return some value these function are simply documenting that there is a parameter and it's an array yet it doesn't explain what needs to be in the array to utilize the function. Same goes with the return values, we are just stating this is an array vs listing out what the array will have in it.
Below is a function list that should continue to grow as we find more. If one needs to be added to the issue description please ping me in IRC. I'm on weekdays for 8+ hours.
Functions:
Comment | File | Size | Author |
---|---|---|---|
#67 | interdiff.txt | 10.34 KB | Albert Volkman |
#65 | fix-undocumented-parameters-1397346-65.patch | 26.56 KB | Albert Volkman |
#59 | fix-undocumented-parameters-1397346-59.patch | 5.92 KB | Albert Volkman |
#55 | fix-undocumented-parameters-1397346-55.patch | 6.3 KB | Albert Volkman |
#55 | interdiff.txt | 490 bytes | Albert Volkman |
Comments
Comment #1
Steven Brown CreditAttribution: Steven Brown commentedAlso, it needs to be defined that if the 'isid' is missing from the array a new image style will be created and if it is then it will be updated.
Comment #2
Steven Brown CreditAttribution: Steven Brown commentedAdded the following patch. This is my first patch for Docs so I know this will need to be finely tuned. Please review. Thank you.
Comment #3
jhodgdonThis seems like a good idea, thanks!
The formatting of the list needs a little work -- see
http://drupal.org/node/1354#lists
Also, Drupal coding standards require us to use "TRUE", not "True" or "true" to refer to boolean TRUE values in documentation and code.
Regarding the documentation itself:
- You need to explain what "isid" is.
- I don't see 'storage' or 'effects' components being used in this particular function. It is just writing to the image_styles table, which only has isid and name fields, and then invoking a hook that doesn't obviously use those other fields either?
- You might mention that is_new won't even be set to a value at all if the style previously existed.
- Generally in Drupal we patch the latest version first (8.x currently), then backport patches to previous version(s) as appropriate.
Comment #4
Steven Brown CreditAttribution: Steven Brown commentedThank you for the review and pointers :).
'storage' is something that gets sent back via the front end ui when you are editing an existing image style. That is why I listed it. Once again I saw that it existed so I listed it. Not sure that it is totally relevant. The same goes with 'effects'. However, the function never uses them so I guess it doesn't matter?
And I just had a brainfart I know better than to not patch against the current branch. My apologies on that.
I will remove the two keys that aren't being used in the function. However, in the return $style array these are presented so shouldn't I leave them there? So the user knows that they will receive that information back as well or?
I'll wait for a response before moving forward. Thanks again!
Comment #5
jhodgdonIn the return value, I would suggest saying something like "The $style parameter, with the following fields added:" or something like that.
And while you're in there, I just noticed that it says "@param style" not "@param $style", so if you could fix that too.. :)
Comment #6
Steven Brown CreditAttribution: Steven Brown commentedComment #7
Steven Brown CreditAttribution: Steven Brown commentedGoing to wait for around a week so we can find as many functions that have this issue and patch it at all together.
Comment #8
kristiaanvandeneyndeAny progress on this issue?
In node_type_save the lack of documentation is really annoying.
The only documentation on the whole special
base => 'node_content'
case is in the examples module.Comment #9
jhodgdon"progress"... What you see here is what has been done. If you want to make a patch, feel free! :)
Comment #10
kristiaanvandeneyndeWell I can't document something I'm looking for information about myself :)
Comment #11
webchickActually, you can, and are often the best person to do so. :) http://www.lullabot.com/articles/drupal-best-practice-document-your-way-...
Comment #12
kristiaanvandeneyndeNice article, Angie!
The problem is that most of the time when I contribute code or figure things out, it's during office hours. And once a problem is fixed or an obstacle tackled, I'm expected to continue to the next. I have, however, convinced my boss of the importance of giving back to the community. Which is why I was (and still am) able to share the code of my first contrib module ManyMail.
But I'm still stuck somewhere between the Loner and the Doer. I've got a teaching heart, but find myself in lack of time to live up to the challenge. Wherever I can within a reasonable amount of time, I do tend to share my previously acquired knowledge though (as seen here).
Finally, if I were to document something, I want to do it right. I find that nothing is more harmful than misinformation, so I'd hate myself for inadvertently sending people down the wrong path.
Which is why I find myself unable to document this issue (so far), because I've got the feeling that I haven't fully grasped the concept of
'base' => 'node content'>/code> yet... :(
Comment #13
Steven Brown CreditAttribution: Steven Brown commentedSorry about the progress. I've been on a project for work that has taken roughly 80 hours of my weeks. So I haven't had much time for anything else. Please do not wait for me to do this.
Comment #14
jhodgdontagging. This issue still needs a new patch.
Comment #15
shiff2kl CreditAttribution: shiff2kl commentedHere is a patch documenting and image_style_save() and node_type_save().
image_style_effects() indicates that it is obsolete in the comments and needs to be removed. Therefore I did not document this function.
Comment #16
jhodgdonimage_style_effects may be marked as obsolete, but it is apparently still being used, so it needs documenting (and especially in D7, where we will port this patch when 8 is done).
The formatting of the lists is better, thanks! One problem:
In that last line, you need to indent "existing style" by two spaces. Same here:
Also, in this area:
The first line here should say:
"The node type to save; an object with the following properties:"
and I think the next lines should have consistent style... Actually "A string." is kind of redundant for the first two, since I think everyone knows a "name" is a string? If you think it needs to say "a string", combine it like the third bullet point, something like "A string giving the machine name...".
And these types of bullet points need more description telling what they are for:
Comment #17
Albert Volkman CreditAttribution: Albert Volkman commentedStyle issues fixed, still needs more details on those fields though.
Comment #18
kid_icarus CreditAttribution: kid_icarus commentedComment #19
jhodgdonComment #17 says "still needs more details", so I'm putting this back to "needs work" until that is addressed.
Comment #20
tim-e CreditAttribution: tim-e commentedComment #21
tim-e CreditAttribution: tim-e commentedFollowing on from comment #17 I have added the additional comments for the function parameters.
Also, some parameters appear to now be obsolete, should I create a new issue/patch for removing these? Im not sure if there is a process for this
Comment #22
tim-e CreditAttribution: tim-e commentedComment #23
kristiaanvandeneyndeLooks good to me
Comment #24
jhodgdonIf parameters are obsolete, that is a code fix and yes it should be a separate issue. Thanks! Actually, file one issue per module/component (image module, node module), if they are in separate components.
Comment #25
jhodgdonRE #23 - did you give this a thorough review and check that all the information in the patch is OK? If so, please set the status to "reviewed and tested by the community". If not, someone needs to do that before the patch can get committed. (i.e., someone needs to verify that all the array elements documented here are correct with respect to the code in those functions)
Comment #26
kristiaanvandeneyndeI can only tell as far as node_type_save goes that the information is correct.
Never worked with image styles before, but the documentation seems quite all right.
Comment #27
jhodgdonI was just about to commit this, but in final review I noticed this at the end of the patch:
This needs to be reformatted as a list: http://drupal.org/node/1354#lists
Comment #28
Albert Volkman CreditAttribution: Albert Volkman commentedListified.
Comment #29
kristiaanvandeneyndeLooks like some lines in there exceed the 80 character limit.
Comment #30
Albert Volkman CreditAttribution: Albert Volkman commentedDocs wrapped to 80 characters.
Comment #31
kristiaanvandeneyndeLooks good to me, let's see what testbot has to say.
Comment #32
jhodgdonHuh, I tried to add a comment to this.. hopefully it didn't go to the wrong issue. :)
So... sorry about #27 -- in looking over this, I realized it is a bit non-standard anyway. We don't normally include the values of CONSTANTS in documentation. So the last bit of this patch should probably just say (and note the added a/the):
A status flag indicating the outcome of the operation, either SAVED_NEW or SAVED_UPDATED.
Comment #33
Albert Volkman CreditAttribution: Albert Volkman commentedHere ya go.
Comment #34
jhodgdonLooks good! Due to there being 4 "avoid commit conflicts" issues for D8 and one for D7 at the moment, I'll probably hold off on committing this though (too much trouble to figure out).
Comment #35
webchickAssigning to Jennifer to indicate that she's on this one.
Comment #36
jhodgdonThis patch has been committed, thanks!
The original report noted that image_style_effects() also needed documentation, and that hasn't been done yet, so setting back to "active" until that is done.
At that point, the patch above + the patch for image_style_effects() will need a backport to 7.x (the current patch does not apply there).
Comment #37
tim-e CreditAttribution: tim-e commentedThe function image_style_effects() is obsolete. Im not sure if there is a process for removing obsolete functions?
Ive attached a patch anyway.
Comment #38
jhodgdonSee comment #16 above as to why we need to document this "obsolete" function that is still being used... Removing must be done on a separate issue.
Comment #39
jhodgdonSee #36 - we still need a patch that fixes the docs for one function (not the patch in #37 that removes the function entirely).
Comment #40
bobbyaldol CreditAttribution: bobbyaldol commentedThe function image_style_effects has been removed from the 8.x version. So this issue needs to be backported to 7.x
Comment #41
bobbyaldol CreditAttribution: bobbyaldol commentedChanged the documentation of image_style_effects as required.
Comment #42
jhodgdonThanks! Can we get the rest of the patch in #33 also added to the D7 patch?
Comment #43
bobbyaldol CreditAttribution: bobbyaldol commented@jhodgdon: Sure, but I am not sure the patch in #41 is right. Have you reviewed it? If not so please do it. So that I can make the necessary modifications as required.
Comment #44
Albert Volkman CreditAttribution: Albert Volkman commentedIt'll be easier for @jhodgdon to review if they're together.
Combined, with some slight adjustments to #33.
Comment #45
kristiaanvandeneyndeNitpicking, so not setting to NW.
Should be
Comment #46
larowlanthis line needs to go (two @return statments)
should be @return array
Comment #47
Albert Volkman CreditAttribution: Albert Volkman commentedThanks for the reviews.
Comment #48
kristiaanvandeneyndeAs a developer, reviewing issues like this are paramount (in my opinion).
Are you sure about this change?
You still write 0 instead of FALSE and I strongly suggest replacing the word 'field' with 'key'.
Fields are something completely different in Drupal than array keys.
Comment #49
Albert Volkman CreditAttribution: Albert Volkman commentedIt looks like that first change was erroneously introduced with #41. Fixed.
My apologies for missing the items in your second suggestion.
Comment #50
jhodgdonThis looks pretty good. A few things to address:
- image_style_save():
Normally we would just say "TRUE if this is a ...", rather than "Is set to...".
- image_style_effects() - needs blank line between @param and @return sections, and a blank line before the @see as well.
- Extra credit if you also fix the verbs in the first lines of these function descriptions. :)
Comment #51
Albert Volkman CreditAttribution: Albert Volkman commentedI went through and found quite a few other issues. Let me know if I went too far and I'll move these to a follow-up issue.
Comment #52
jhodgdonWell, we might have to bump this issue back to d8 if you want to do all of that... or at least verify those changes are already in d8. Probably best to stick to what has already been done for d8.
Comment #53
Albert Volkman CreditAttribution: Albert Volkman commentedI don't mind going the extra mile to make sure its right :)
Comment #54
jhodgdonThis mostly looks good, thanks!
I only saw one thing that needs fixing, in image.module (not sure what function it is):
This should probably be: "Page callback: Generates a derivative, given a style and image path."
Comment #55
Albert Volkman CreditAttribution: Albert Volkman commentedFixed.
Comment #56
kristiaanvandeneyndeJust make sure that you put the previous work in the D7 version too.
This issue is getting confusing :)
Comment #57
jhodgdonLooks good! I'll get this committed to 8.x shortly, and then we can get back to the 7.x patch.
Comment #58
jhodgdonuh oh, the patch no longer applies.
Comment #59
Albert Volkman CreditAttribution: Albert Volkman commentedRe-roll.
Comment #60
jhodgdonLooks fine. I'll wait for the test bot (which is delayed right now since d8 HEAD was broken for a while today) and then get it committed. Thanks!
Comment #62
jhodgdon#59: fix-undocumented-parameters-1397346-59.patch queued for re-testing.
Comment #63
jhodgdonThe failure was unrelated to this patch (Anonymous name found in user_autocomplete test).
Comment #64
jhodgdonOK, the patch in #59 is committed to 8.x We can go back to the 7.x patch now.
Comment #65
Albert Volkman CreditAttribution: Albert Volkman commentedHere we go. Interdiff'd against #51.
Comment #66
Albert Volkman CreditAttribution: Albert Volkman commentedComment #67
Albert Volkman CreditAttribution: Albert Volkman commentedDefinitely the wrong interdiff.
Comment #68
jhodgdonThanks! I think the 7.x patch is just fine, and I'll get it committed shortly.
Comment #69
jhodgdonCommitted to 7.x. Thanks to all who participated!
Comment #70.0
(not verified) CreditAttribution: commentedChanged the issue due to finding several functions that have the same issue.
Comment #71
Andrew Answer CreditAttribution: Andrew Answer as a volunteer commented