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:

Files: 
CommentFileSizeAuthor
#67 interdiff.txt10.34 KBAlbert Volkman
#65 fix-undocumented-parameters-1397346-65.patch26.56 KBAlbert Volkman
PASSED: [[SimpleTest]]: [MySQL] 39,692 pass(es).
[ View ]
#59 fix-undocumented-parameters-1397346-59.patch5.92 KBAlbert Volkman
PASSED: [[SimpleTest]]: [MySQL] 50,604 pass(es).
[ View ]
#55 fix-undocumented-parameters-1397346-55.patch6.3 KBAlbert Volkman
PASSED: [[SimpleTest]]: [MySQL] 50,983 pass(es).
[ View ]
#55 interdiff.txt490 bytesAlbert Volkman
#53 fix-undocumented-parameters-1397346-53.patch6.3 KBAlbert Volkman
PASSED: [[SimpleTest]]: [MySQL] 50,879 pass(es).
[ View ]
#51 fix-undocumented-parameters-1397346-51.patch18.49 KBAlbert Volkman
PASSED: [[SimpleTest]]: [MySQL] 39,922 pass(es).
[ View ]
#51 interdiff.txt16.32 KBAlbert Volkman
#49 fix-undocumented-parameters-1397346-49.patch3.63 KBAlbert Volkman
PASSED: [[SimpleTest]]: [MySQL] 39,722 pass(es).
[ View ]
#49 interdiff.txt1.2 KBAlbert Volkman
#47 fix-undocumented-parameters-1397346-47.patch3.71 KBAlbert Volkman
PASSED: [[SimpleTest]]: [MySQL] 39,788 pass(es).
[ View ]
#47 interdiff.txt1.72 KBAlbert Volkman
#44 fix-undocumented-parameters-1397346-44.patch3.59 KBAlbert Volkman
PASSED: [[SimpleTest]]: [MySQL] 39,847 pass(es).
[ View ]
#44 interdiff.txt2.91 KBAlbert Volkman
#41 fix-undocumented-parameters-1397346-41.patch863 bytesbobbyaldol
PASSED: [[SimpleTest]]: [MySQL] 39,682 pass(es).
[ View ]
#37 1397346-fix-undocumented-parameters.patch3 KBtim-e
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1397346-fix-undocumented-parameters.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#33 documented_parameters-1397346-33.patch2.9 KBAlbert Volkman
PASSED: [[SimpleTest]]: [MySQL] 41,096 pass(es).
[ View ]
#33 interdiff.txt629 bytesAlbert Volkman
#30 documented_parameters-1397346-30.patch2.89 KBAlbert Volkman
PASSED: [[SimpleTest]]: [MySQL] 41,091 pass(es).
[ View ]
#28 documented_parameters-1397346-28.patch2.83 KBAlbert Volkman
PASSED: [[SimpleTest]]: [MySQL] 40,381 pass(es).
[ View ]
#28 interdiff.txt589 bytesAlbert Volkman
#21 1397346-add-documentation-functions.patch2.83 KBtim-e
PASSED: [[SimpleTest]]: [MySQL] 40,361 pass(es).
[ View ]
#17 documented_parameters-1397346-17.patch2.24 KBAlbert Volkman
PASSED: [[SimpleTest]]: [MySQL] 40,738 pass(es).
[ View ]
#17 interdiff.txt1.43 KBAlbert Volkman
#15 documented_parameters-1397346-15.patch2.17 KBshiff2kl
PASSED: [[SimpleTest]]: [MySQL] 39,745 pass(es).
[ View ]
#2 style_definition_array-1397346-2.patch1.38 KBFatGuyLaughing
PASSED: [[SimpleTest]]: [MySQL] 37,284 pass(es).
[ View ]

Comments

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

Status:Active» Needs review
StatusFileSize
new1.38 KB
PASSED: [[SimpleTest]]: [MySQL] 37,284 pass(es).
[ View ]

Added the following patch. This is my first patch for Docs so I know this will need to be finely tuned. Please review. Thank you.

Version:7.x-dev» 8.x-dev
Issue tags:+needs backport to D7

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

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

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

Title:image_style_save style array definitionFix undocumented parameters and return values

Status:Needs review» Needs work

Going to wait for around a week so we can find as many functions that have this issue and patch it at all together.

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

<?php
// 'base' tells Drupal the base string for hook functions.
// This is often the module name; if base is set to 'mymodule', Drupal
// would call mymodule_insert() or similar for node hooks.
// In this case, we set base equal to 'node_content' so Drupal will handle
// our node as if we had designed it in the UI.
'base' => 'node_content',
?>

"progress"... What you see here is what has been done. If you want to make a patch, feel free! :)

Well I can't document something I'm looking for information about myself :)

Actually, you can, and are often the best person to do so. :) http://www.lullabot.com/articles/drupal-best-practice-document-your-way-...

Nice 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... :(

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

Issue tags:+Novice

tagging. This issue still needs a new patch.

Status:Needs work» Needs review
StatusFileSize
new2.17 KB
PASSED: [[SimpleTest]]: [MySQL] 39,745 pass(es).
[ View ]

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

Title:Fix undocumented parameters and return valuesFix undocumented parameters and return values in a few functions
Status:Needs review» Needs work

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

+ * @return array
+ *   An image style array containing:
+ *   - name: An unique name for the style.
+ *   - effects: An array of effects.
+ *   - is_new: Is set to TRUE if this is a new style, and FALSE if it is an
+ *   existing style.
  */
function image_style_save($style) {

In that last line, you need to indent "existing style" by two spaces. Same here:
+ *   - base: A string that indicates the base string for hook functions. For
+ *   example, 'node_content' is the value used by the UI when creating a new
+ *   node type.

Also, in this area:

*   The node type to save, as an object.
+ *   - type: A string. The machine name of the node type.
+ *   - name: A string. The human-readable name of the node type.
+ *   - base: A string that indicates the base string for hook functions. For

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:

+ *   - help: A string.
+ *   - custom: An integer.

StatusFileSize
new1.43 KB
new2.24 KB
PASSED: [[SimpleTest]]: [MySQL] 40,738 pass(es).
[ View ]

Style issues fixed, still needs more details on those fields though.

Status:Needs work» Needs review

Status:Needs review» Needs work

Comment #17 says "still needs more details", so I'm putting this back to "needs work" until that is addressed.

Assigned:Unassigned» tim-e

StatusFileSize
new2.83 KB
PASSED: [[SimpleTest]]: [MySQL] 40,361 pass(es).
[ View ]

Following 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

Status:Needs work» Needs review

Looks good to me

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

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

Status:Needs review» Reviewed & tested by the community

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

Status:Reviewed & tested by the community» Needs work

I was just about to commit this, but in final review I noticed this at the end of the patch:

+ * @return int
  *   Status flag indicating outcome of the operation.
+ *   A value of 1 indicates SAVED_NEW
+ *   A value of 2 indicates SAVED_UPDATED

This needs to be reformatted as a list: http://drupal.org/node/1354#lists

Status:Needs work» Needs review
StatusFileSize
new589 bytes
new2.83 KB
PASSED: [[SimpleTest]]: [MySQL] 40,381 pass(es).
[ View ]

Listified.

Status:Needs review» Needs work

Looks like some lines in there exceed the 80 character limit.

Comments should be word-wrapped if the line length would exceed 80 characters (i.e., go past the 80th column, including any leading spaces and comment characters in the 80 character count). They should be as long as possible within the 80-character limit.

StatusFileSize
new2.89 KB
PASSED: [[SimpleTest]]: [MySQL] 41,091 pass(es).
[ View ]

Docs wrapped to 80 characters.

Status:Needs work» Needs review

Looks good to me, let's see what testbot has to say.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new629 bytes
new2.9 KB
PASSED: [[SimpleTest]]: [MySQL] 41,096 pass(es).
[ View ]

Here ya go.

Status:Needs review» Reviewed & tested by the community

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

Assigned:tim-e» jhodgdon

Assigning to Jennifer to indicate that she's on this one.

Status:Reviewed & tested by the community» Active

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

StatusFileSize
new3 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1397346-fix-undocumented-parameters.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

The function image_style_effects() is obsolete. Im not sure if there is a process for removing obsolete functions?

Ive attached a patch anyway.

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

Assigned:jhodgdon» Unassigned

See #36 - we still need a patch that fixes the docs for one function (not the patch in #37 that removes the function entirely).

Version:8.x-dev» 7.x-dev
Assigned:Unassigned» bobbyaldol

The function image_style_effects has been removed from the 8.x version. So this issue needs to be backported to 7.x

Status:Active» Needs review
StatusFileSize
new863 bytes
PASSED: [[SimpleTest]]: [MySQL] 39,682 pass(es).
[ View ]

Changed the documentation of image_style_effects as required.

Status:Needs review» Needs work

Thanks! Can we get the rest of the patch in #33 also added to the D7 patch?

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

Status:Needs work» Needs review
StatusFileSize
new2.91 KB
new3.59 KB
PASSED: [[SimpleTest]]: [MySQL] 39,847 pass(es).
[ View ]

It'll be easier for @jhodgdon to review if they're together.

Combined, with some slight adjustments to #33.

Nitpicking, so not setting to NW.

+ *   - orig_type: A string giving the original machine-readable name of this
+ *     node type. This may be different from the current type name if the locked
+ *     field is 0.

Should be

+ *   - orig_type: A string giving the original machine-readable name of this
+ *     node type. This may be different from the current type name if the 'locked'
+ *     key is set to FALSE.

Status:Needs review» Needs work

+++ b/modules/image/image.moduleundefined
@@ -656,8 +656,19 @@ function image_style_load($name = NULL, $isid = NULL, $include = NULL) {
+ *
  * @return
  *   An image style array. In the case of a new style, 'isid' will be populated.
  */

this line needs to go (two @return statments)

+++ b/modules/image/image.moduleundefined
@@ -713,12 +724,14 @@ function image_style_delete($style, $replacement_style_name = '') {
+ *   - isid: The unique image style ID that contains this image effect.
  * @return

should be @return array

Status:Needs work» Needs review
StatusFileSize
new1.72 KB
new3.71 KB
PASSED: [[SimpleTest]]: [MySQL] 39,788 pass(es).
[ View ]

Thanks for the reviews.

Status:Needs review» Needs work

As a developer, reviewing issues like this are paramount (in my opinion).

- *   format array('isid' => array()), or an empty array if the specified style
+ *   format array('ieid' => array()), or an empty array if the specified style
Are you sure about this change?

+ *   - orig_type: A string giving the original machine-readable name of this
+ *     node type. This may be different from the current type name if the
+ *     'locked' field is 0.
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.

Status:Needs work» Needs review
StatusFileSize
new1.2 KB
new3.63 KB
PASSED: [[SimpleTest]]: [MySQL] 39,722 pass(es).
[ View ]

It looks like that first change was erroneously introduced with #41. Fixed.

My apologies for missing the items in your second suggestion.

Status:Needs review» Needs work

This looks pretty good. A few things to address:

- image_style_save():

+ *   - is_new: Is set to TRUE if this is a new style, and FALSE if it is an
+ *     existing style.

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

Status:Needs work» Needs review
StatusFileSize
new16.32 KB
new18.49 KB
PASSED: [[SimpleTest]]: [MySQL] 39,922 pass(es).
[ View ]

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

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

Version:7.x-dev» 8.x-dev
StatusFileSize
new6.3 KB
PASSED: [[SimpleTest]]: [MySQL] 50,879 pass(es).
[ View ]

I don't mind going the extra mile to make sure its right :)

Status:Needs review» Needs work

This mostly looks good, thanks!

I only saw one thing that needs fixing, in image.module (not sure what function it is):

* Menu callback: Given a style and image path, generate a derivative.

This should probably be: "Page callback: Generates a derivative, given a style and image path."

Status:Needs work» Needs review
StatusFileSize
new490 bytes
new6.3 KB
PASSED: [[SimpleTest]]: [MySQL] 50,983 pass(es).
[ View ]

Fixed.

Just make sure that you put the previous work in the D7 version too.
This issue is getting confusing :)

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

Looks good! I'll get this committed to 8.x shortly, and then we can get back to the 7.x patch.

Assigned:jhodgdon» Unassigned
Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs reroll

uh oh, the patch no longer applies.

Status:Needs work» Needs review
StatusFileSize
new5.92 KB
PASSED: [[SimpleTest]]: [MySQL] 50,604 pass(es).
[ View ]

Re-roll.

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

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

Status:Reviewed & tested by the community» Needs work
Issue tags:-Novice, -needs backport to D7, -Needs reroll

The last submitted patch, fix-undocumented-parameters-1397346-59.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+Novice, +needs backport to D7, +Needs reroll

Status:Needs review» Reviewed & tested by the community

The failure was unrelated to this patch (Anonymous name found in user_autocomplete test).

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

OK, the patch in #59 is committed to 8.x We can go back to the 7.x patch now.

StatusFileSize
new26.56 KB
PASSED: [[SimpleTest]]: [MySQL] 39,692 pass(es).
[ View ]

Here we go. Interdiff'd against #51.

Status:Patch (to be ported)» Needs review

StatusFileSize
new10.34 KB

Definitely the wrong interdiff.

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

Thanks! I think the 7.x patch is just fine, and I'll get it committed shortly.

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

Committed to 7.x. Thanks to all who participated!

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

Issue summary:View changes

Changed the issue due to finding several functions that have the same issue.