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:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Steven Brown’s picture

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.

Steven Brown’s picture

Status: Active » Needs review
FileSize
1.38 KB

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.

jhodgdon’s picture

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.

Steven Brown’s picture

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!

jhodgdon’s picture

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

Steven Brown’s picture

Title: image_style_save style array definition » Fix undocumented parameters and return values
Steven Brown’s picture

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.

kristiaanvandeneynde’s picture

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.

// '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',
jhodgdon’s picture

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

kristiaanvandeneynde’s picture

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

webchick’s picture

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

kristiaanvandeneynde’s picture

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

Steven Brown’s picture

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.

jhodgdon’s picture

Issue tags: +Novice

tagging. This issue still needs a new patch.

shiff2kl’s picture

Status: Needs work » Needs review
FileSize
2.17 KB

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.

jhodgdon’s picture

Title: Fix undocumented parameters and return values » Fix 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.
Albert Volkman’s picture

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

kid_icarus’s picture

Status: Needs work » Needs review
jhodgdon’s picture

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.

tim-e’s picture

Assigned: Unassigned » tim-e
tim-e’s picture

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

tim-e’s picture

Status: Needs work » Needs review
kristiaanvandeneynde’s picture

Looks good to me

jhodgdon’s picture

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.

jhodgdon’s picture

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)

kristiaanvandeneynde’s picture

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.

jhodgdon’s picture

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

Albert Volkman’s picture

Status: Needs work » Needs review
FileSize
589 bytes
2.83 KB

Listified.

kristiaanvandeneynde’s picture

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.

Albert Volkman’s picture

Docs wrapped to 80 characters.

kristiaanvandeneynde’s picture

Status: Needs work » Needs review

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

jhodgdon’s picture

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.

Albert Volkman’s picture

Status: Needs work » Needs review
FileSize
629 bytes
2.9 KB

Here ya go.

jhodgdon’s picture

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

webchick’s picture

Assigned: tim-e » jhodgdon

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

jhodgdon’s picture

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

tim-e’s picture

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

Ive attached a patch anyway.

jhodgdon’s picture

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.

jhodgdon’s picture

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

bobbyaldol’s picture

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

bobbyaldol’s picture

Status: Active » Needs review
FileSize
863 bytes

Changed the documentation of image_style_effects as required.

jhodgdon’s picture

Status: Needs review » Needs work

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

bobbyaldol’s picture

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

Albert Volkman’s picture

Status: Needs work » Needs review
FileSize
2.91 KB
3.59 KB

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

Combined, with some slight adjustments to #33.

kristiaanvandeneynde’s picture

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.
larowlan’s picture

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

Albert Volkman’s picture

Status: Needs work » Needs review
FileSize
1.72 KB
3.71 KB

Thanks for the reviews.

kristiaanvandeneynde’s picture

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.

Albert Volkman’s picture

Status: Needs work » Needs review
FileSize
1.2 KB
3.63 KB

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

My apologies for missing the items in your second suggestion.

jhodgdon’s picture

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

Albert Volkman’s picture

Status: Needs work » Needs review
FileSize
16.32 KB
18.49 KB

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.

jhodgdon’s picture

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.

Albert Volkman’s picture

Version: 7.x-dev » 8.x-dev
FileSize
6.3 KB

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

jhodgdon’s picture

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

Albert Volkman’s picture

Status: Needs work » Needs review
FileSize
490 bytes
6.3 KB

Fixed.

kristiaanvandeneynde’s picture

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

jhodgdon’s picture

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.

jhodgdon’s picture

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

uh oh, the patch no longer applies.

Albert Volkman’s picture

Status: Needs work » Needs review
FileSize
5.92 KB

Re-roll.

jhodgdon’s picture

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.

jhodgdon’s picture

Status: Needs work » Needs review
Issue tags: +Novice, +Needs backport to D7, +Needs reroll
jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

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

jhodgdon’s picture

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.

Albert Volkman’s picture

Here we go. Interdiff'd against #51.

Albert Volkman’s picture

Status: Patch (to be ported) » Needs review
Albert Volkman’s picture

FileSize
10.34 KB

Definitely the wrong interdiff.

jhodgdon’s picture

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.

jhodgdon’s picture

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.

Anonymous’s picture

Issue summary: View changes

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

Andrew Answer’s picture

Issue summary: View changes
Issue tags: -Needs backport to D7, -Needs reroll