hook_field_storage_load() last param not documented

Good catch! It does indeed look like the implementations of hook_field_storage_load() use a $options parameter, and this is not on the hook doc page. The hook is invoked from field_attach_load(), which is definitely passing in $options.

See
http://api.drupal.org/api/function/hook_field_storage_load/7
http://api.drupal.org/api/function/field_sql_storage_field_storage_load/7
http://api.drupal.org/api/function/field_attach_load/7

This is also true of hook_field_storage_pre_load().
field_attach_load() calls it with the $options param, which is not documented in the hook's PHPdoc
http://api.drupal.org/api/function/hook_field_storage_pre_load/7

Adds docs for missing params and fixes a couple formatting errors (80 chars wrapping, indentation)

The test failure looks like something unrelated to this patch.

That aside, a couple of things to fix in the patch:
a) Need a blank line between @param section and @return in all functions. See http://drupal.org/node/1354

b)

+ *   - 'deleted': If TRUE, deleted field should be returned as well as
+ *     non-deleted fields. If unset or FALSE, only non-deleted fields should be
+ *     returned.

deleted field -> deleted fields ? This occurs in a couple of functions.

c)

  * @return
- *   Loaded field values are added to $entities. Fields with no values should be
- *   set as an empty array.
+ *   Loaded field values are added to $entities. Fields with no values should
+ *   be set as an empty array.

This doesn't look like it describes the return value -- what is the return value, or is there a return value? And what is this referring to really?

This applies to the next function as well.

d)

 *   An array keyed by field ids whose data has already been loaded and
 *   therefore should not be loaded again. The values associated to these keys
 *   are not specified.

ids -> IDs ... or maybe it should be ID without the s?
associated to -> associated with

a) all functions under modules/field/ currently skip that convention. Added the empty line for the functions touched in the patch, but adjusting the remaining 99,9% is out of scope here ;-).

b) fixed

c) Those functions only act by altering the incoming params by reference. Documenting those expected side-effects in the @return section is, AFAIK, common practice in the rest of core, but I might be proven wrong.
Here also, this is used in many other places in field module.

d) 'ids' is also used all over the place in field module, out of scope for this patch.

Ummm...

- @return is for return values, period. Use the main section of the doc to describe what the function does, if there is no return value.
- "ids" violates standard English usage.

I realize that there is a lot of doc in core that is in violation of our coding and style standards. The fact that a violation of coding standards is "common practice" is not really a good excuse for committing doc patches that leave the doc out of compliance with our standards. I'm not asking for anyone to go and fix an entire file's worth of functions, but at least when we are touching the doc of particular functions, we can bring them into compliance. Please? :)

I don't agree, esp on the '@return is for return values, period. Use the main section of the doc to describe what the function does, if there is no return value' item. This obfuscates the "contract", which lies in the @param, @result sections. If @return is not the place to document side effects (params altered by ref, exceptions...), then there should be another @section dedicated for this.

However, you're the boss :-).

It's not that I'm the boss. It's that our coding standards, based on industry standards, http://drupal.org/node/1354#functions say:

After all the parameters, a @return directive should be used to document the return value if there is one. There should be a blank line between the @param section and @return directive.
If there is no return value, omit the @return directive entirely.

I would say the side effect here should also be noted on the entities @param, because that is what is being altered, correct?

That aside, this doc needs some English style/grammar/usage/clarity editing, for things like "need to added" and "entity type of entity". I will take a try at rewriting it on Monday, unless someone gets to it before me.

Here's a patch that fixes up the doc, I hope.

+++ modules/field/field.api.php	29 Mar 2010 15:26:57 -0000
@@ -1344,27 +1349,34 @@
- * This lets 3rd party modules override, mirror, shard, or otherwise store a
+ * This lets 3rd party modules override, mirror, share, or otherwise store a

"shard" is the intended word here.
http://en.wikipedia.org/wiki/Shard_%28database_architecture%29

Other than that, RTBC.

Powered by Dreditor.

Ah. Little did I know...

It looks like "shard" is mostly a noun, not a verb, though. So should it be "lets modules override, mirror, use a shard, or ..." ?

This is not my comment, I didn't know that term either before bjaspan explained it to me :-).
That same page mentions 'database sharding', and a google search on this expression brings 47 000 answers, so i guess the verb is ok too.

Google would treat "shard" "shards" "sharding" etc. as the same term for purposes of searching, by the way. So if you typed in "sharding" that would also bring up pages that only used it as the noun shard/shards, and having 47000 results for "sharding" doesn't conclusively mean it's a verb.

But I do see "sharding" on the Wikipedia page, although it's just used as a gerund-like for the action of making a shard and not technically as a verb... But fine. Guess that needs to get changed back.

re google # of results : Don't want to sound anal, but I don't think it's true if you use exact search of an expression with double quotes.

I sit corrected.

Rerolled #10 without the 'shard' / 'share' replace.

You know... we should probably say "loaded" instead of "returned in this section at the end of the first hook:

+ * @param $options
+ *   An associative array of additional options, with the following keys:
+ *   - 'deleted': If TRUE, deleted fields should be returned as well as
+ *     non-deleted fields. If unset or FALSE, only non-deleted fields should be
+ *     returned.

And here in the second hook:

+ *   - 'deleted': If TRUE, deleted fields should be returned as well as
+ *     non-deleted fields. If unset or FALSE, only non-deleted fields should be
+ *     returned.

And in field_attach_load(), we should probably add a note in @param $entities that says the fields are loaded directly there, even though it's stated above...

Done.
Also updated field_attach_revisions() phpdoc accordingly, and fixed 80 chars wrapping over there.

Excellent work!

Committed to CVS HEAD.