Field Attach API page has outdated information

I don't think this is much of a Novice issue...

I think what you are saying is that on that page, where it says:

Fieldable types call Field Attach API functions during their own API calls; for example, node_load() calls field_attach_load(). A fieldable type is not required to use all of the Field Attach API functions.

This is definitely incorrect -- node_load() doesn't call field_attach_load() at all...

So should it instead say something like:

Fieldable types using the entity loading blah blah blah should set the 'fieldable' component of their hook_entity_info() implementation to TRUE. This will cause field_attach_load() to be called when the object is loaded from the database.

(NOTE: not sure if the above is correct -- I'm asking -- I'm totally unfamiliar with the new entity and field stuff, at least so far)

And just as a note, the doc for that page is located in modules/field/field.attach.inc

Here's a patch. Hopefully I've understood this correctly - definitely needs a review.

#3: 584130.patch queued for re-testing.

add tag

Looks like this patch needs rerolling...

Here's a re-roll.

+++ modules/field/field.attach.inc	16 Mar 2010 18:25:34 -0000
@@ -80,21 +80,21 @@
+ * bundle, if any, is read from the entity's 'bundle keys' property

should actually be "the entity's 'bundle' property, as defined in the 'object keys' section of hook_entity_info()."

+++ modules/field/field.attach.inc	16 Mar 2010 18:25:34 -0000
@@ -80,21 +80,21 @@
+ * Entities using the standard entity loading mechanism should set the
+ * 'fieldable' property in hook_entity_info() to TRUE. This will cause
+ * field_attach_load() or field_attach_load_revision() to be called when the
+ * entity is loaded, if the default entity controller class is used.

I'd rather say : "Fieldable entity types need to set the 'fieldable' property in hook_entity_info() to TRUE. [the rest is correct: ] This will cause field_attach_load() or field_attach_load_revision() to be called when the entity is loaded, if the default entity controller class is used."

Powered by Dreditor.

Ah, got it. Thanks!
How's this patch?

+++ modules/field/field.attach.inc	16 Mar 2010 23:52:59 -0000
@@ -80,21 +80,22 @@
+ * bundle, if any, is read from the entity's 'bundle' object key, in the
+ * 'object keys' element of the hook_entity_info() return value for
+ * $entity_type.

Nitpick : I think "the hook_entity_info() return value for the $entity_type" is overly accurate, to the detriment of clarity. "the 'object keys' entry in hook_entity_info()" is IMO good enough, and less convoluted.

Aside from that, the sentence needs to be something like "... is read from the entity's 'bundle' property (e.g. $node->type, or $taxonomy_term->vocabulary_machine_name), *as defined in* the 'object keys' entry (... whatever you pick for the rest of the sentence ...)".

Meaning : $entity_info['object keys']['bundle'] provides the name of the $entity->property that holds the bundle name.

145 critical left. Go review some!

OK... I decided to expand this a bit. Hopefully I have it right this time.

Reworded a bit, and fixed 80 chars wrapping in this doc block.

Oops. Fixes 80 chars wrapping in the newly added text.

Latest patch has an indentation glitch:

+ * to individual entities.
+*
+ * Field Attach API functions generally take $entity_type and $entity arguments

Other minor suggestions:

+ * Field Attach API uses the concept of bundles: the set of fields for a given

This should start with "The".

Not good wrapping:

+ * the set of fields of a node is determined by the node type.
+ * Field API reads the bundle name for a given entity from a particular
+ * property of the entity object, and hook_entity_info() defines which property

If this is all one paragraph, and I think it is, then some of the text below needs to be moved up -- it should always wrap at nearly 80 characters. This needs to be fixed at multiple spots in the patch.

Also, "Field API" should be "The Field API" in several places. If it is being used as a noun, it should have "the". If it is being used as an adjective, such as "Field API functions", then it should not have "the".

+ * field_attach_load() is automatically called by the default entity controller
+ * class, and thus, in most cases, doesn't need to be explicitly called by the
+ * entity type module explicitly.

Should not have "explicitly" twice. This occurs in the next function as well.

Right, fixed.

Amost there... Wrapping problem remaining:

+ *       the entity. The Field API assumes that all revision ids are unique
+ *       across all entities of a type.
+ *       This element can be omitted if the entities of this type are not

And the next @param below has the same wrapping problem.

Other than that, looks great!

Right. I guess those were originally intended as line breaks...
Fixed.

Looks good. As you probably know, we cannot have line breaks within a @param, as @param is a paragraph-level directive (so a line break ends it).

Yes, and that's a pretty annoying limitation, for that matter ;-). Some @params could use a couple nicely formatted paragraphs instead of a single block of text. Ah, life sucks...

Hmmm... Maybe we should file a feature request on the API module? We could perhaps put a @paragraph or @break or some kind of marker like that on an otherwise blank line, to indicate we need a paragraph break in the middle of a @param or @return. I doubt it would be too difficult to implement... Thoughts?

Great! Committed to CVS.

@yched: I filed a feature request for the API module to allow breaks --
#762610: Make a way to allow line breaks in @param, @return