Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
http://api.drupal.org/api/group/field_attach/7 incorrectly states that each fieldable entity needs to call field_attach_load, when in fact the unified entity loading stuff from entity_load no longer requires that to be called explicitly.
Comment | File | Size | Author |
---|---|---|---|
#19 | field_attach_doc-584130-19.patch | 9.48 KB | yched |
#17 | field_attach_doc-584130-17.patch | 8.46 KB | yched |
#15 | field_attach_doc-584130-15.patch | 8.48 KB | yched |
#14 | field_attach_doc-584130-14.patch | 8.48 KB | yched |
#13 | 584130new.patch | 2.83 KB | jhodgdon |
Comments
Comment #1
mikey_p CreditAttribution: mikey_p commentedtaggin
Comment #2
jhodgdonI don't think this is much of a Novice issue...
I think what you are saying is that on that page, where it says:
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
Comment #3
jhodgdonHere's a patch. Hopefully I've understood this correctly - definitely needs a review.
Comment #5
jhodgdon#3: 584130.patch queued for re-testing.
Comment #6
moshe weitzman CreditAttribution: moshe weitzman commentedadd tag
Comment #8
jhodgdonLooks like this patch needs rerolling...
Comment #9
jhodgdonHere's a re-roll.
Comment #10
yched CreditAttribution: yched commentedshould actually be "the entity's 'bundle' property, as defined in the 'object keys' section of hook_entity_info()."
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.
Comment #11
jhodgdonAh, got it. Thanks!
How's this patch?
Comment #12
yched CreditAttribution: yched commentedNitpick : 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!
Comment #13
jhodgdonOK... I decided to expand this a bit. Hopefully I have it right this time.
Comment #14
yched CreditAttribution: yched commentedReworded a bit, and fixed 80 chars wrapping in this doc block.
Comment #15
yched CreditAttribution: yched commentedOops. Fixes 80 chars wrapping in the newly added text.
Comment #16
jhodgdonLatest patch has an indentation glitch:
Other minor suggestions:
This should start with "The".
Not good wrapping:
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".
Should not have "explicitly" twice. This occurs in the next function as well.
Comment #17
yched CreditAttribution: yched commentedRight, fixed.
Comment #18
jhodgdonAmost there... Wrapping problem remaining:
And the next @param below has the same wrapping problem.
Other than that, looks great!
Comment #19
yched CreditAttribution: yched commentedRight. I guess those were originally intended as line breaks...
Fixed.
Comment #20
jhodgdonLooks 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).
Comment #21
yched CreditAttribution: yched commentedYes, 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...
Comment #22
jhodgdonHmmm... 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?
Comment #23
Dries CreditAttribution: Dries commentedGreat! Committed to CVS.
Comment #24
jhodgdon@yched: I filed a feature request for the API module to allow breaks --
#762610: Make a way to allow line breaks in @param, @return