from scott falconer:
$options['request'] should say that it's a string in param=value&param=value&... format, which isn't exactly clear from the current doc.

from jhodgdon:
The list section of the $options is not formatted according to the standards on http://drupal.org/1354

CommentFileSizeAuthor
#12 950662-5.patch3.88 KBamateescu
#8 950662-4.patch3.76 KBamateescu
#7 950662-3.patch3.79 KBamateescu
#4 950662-2.patch3.81 KBamateescu
#2 950662.patch3.77 KBamateescu

Comments

scott falconer’s picture

scott falconer commented

Subscribe.

amateescu’s picture

amateescu commented
Status: Active » Needs work
StatusFileSize
new 950662.patch3.77 KB

First try. I couldn't find $options['request'] so this patch is addressing only jhodgdon's comment.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

Sorry. It's the request component of the $options argument that we were referring to.

Also, in lists, it is preferred *not* to put the components in quotes. "The keys should not be put in quotes unless they contain colons (which is unlikely)." (from http://drupal.org/node/1354#lists). I realize many many places in the drupal source code have quotes, but that's our current recommendation.

Otherwise, that's a good start...

amateescu’s picture

amateescu commented
Status: Needs work » Needs review
StatusFileSize
new 950662-2.patch3.81 KB

Ok, second try.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Needs work

Oops, my mistake. The new text should be part of the data component of $options. Sorry!

Also, as long as we are fixing things:
An array which can have one or more of the following keys:
==>
An array that can have one or more of the following elements:

Also, I don't think (three) is necessary in:
Defaults to 3 (three).

amateescu’s picture

amateescu commented
Status: Needs work » Needs review
StatusFileSize
new 950662-3.patch3.79 KB

Done. I'm not sure about the quotes added to the example string, but without them the sentence would end with "....".

amateescu’s picture

amateescu commented
StatusFileSize
new 950662-4.patch3.76 KB

Removed 'array' from $options param.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Needs work

Why did you remove 'array' from the $options param? That was correct, actually. And the @return could say @return object as well.

Missing:

An array which can have one or more of the following keys:
==>
An array that can have one or more of the following elements:

Also, an object doesn't have "parameters", so

An object which can have one or more of the following parameters:
==>
An object that can have one or more of the following components:

(In both cases, "which" -> "that", and the last word needs to change.)

amateescu’s picture

amateescu commented

I removed 'array' because @yched told me to in another issue: http://drupal.org/node/948116#comment-3616214

I will upload a new patch in a few hours when I get home.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

yched was wrong.
http://drupal.org/node/1354#functions

amateescu’s picture

amateescu commented
StatusFileSize
new 950662-5.patch3.88 KB

I can see that now.. new patch attached.

amateescu’s picture

amateescu commented
Status: Needs work » Needs review
jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

Looks good, thanks for all the iterations!

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

Status: Fixed » Closed (fixed)

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