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.There are several hooks in modules/field/field.api.php that do not have function body examples to go with them. They need to be filled out.
There's also one hook in field_ui.api,php that I don't think is ever invoked (at least it's not obvious to me where):
hook_field_formatter_settings_form()
If it is a real hook, in needs a function body. If it isn't, it should be removed.
| Comment | File | Size | Author |
|---|---|---|---|
| #54 | 675116-clean-up-field-documentation.patch | 17.02 KB | Damien Tournoud |
| #45 | 675116-45.patch | 23.64 KB | jhodgdon |
| #27 | 675116-somebodies.patch | 23.63 KB | jhodgdon |
| #20 | 675116-fieldapi-v2.patch | 46.02 KB | jhodgdon |
| #11 | 675116-field-api.patch | 43.01 KB | jhodgdon |
Comments
Comment #1
yched CreditAttribution: yched commentedhook_field_formatter_settings_form() is not invoked currently, because so far no good UI solution was found for formatter settings :-(.
Comment #2
jhodgdonIt also looks like hook_field_update_instance() is not ever called, so it should be removed? That should be verified...
Comment #3
gddAnother problem in field.api.php is that many of the hooks don't indicate that their params should be passed by reference. These needs to be fixed as well.
Comment #4
jhodgdonJust as a note on #3, this probably mostly applies to _alter hooks. Regular hooks generally return their information, whereas alter hooks alter existing information and usually that info has to be passed by reference into the hook function.
Comment #5
fagoAd #2, yep I think that's a bug. See #739470: hook_field_update_instance() invocation is missing.
Comment #6
jhodgdonBump. This is the only unpatched issue left to get all the new hooks documented in D7 (barring people finding more issues with the other hook doc).
Anyone want to take it on? :)
Comment #7
jhodgdonOK, I'll do it.
Comment #8
jhodgdonHere's a patch for field_ui.api.php
Still need to do a separate patch for field.api.php
EDIT (added): Note that I removed that hook that wasn't being called anywhere. If someone figures out how to use it, they can add it back in then. For now we don't want nonexistent hooks documented...
Comment #9
sunLovely.
Comment #10
jhodgdonDoes that mean you want to mark it RTBC?
And by the way, separate patch for field.api.php is still to come.
Comment #11
jhodgdonNote on field.api.php - the prepare_translation hook section is being worked on separately:
#362021: field_attach_prepare_translation needs to be updated for D7 API
So I decided to split this up further.... I did some (actually a lot of) cleanup on the doc in field.api.php, and put @todo tags in the function bodies for all hooks that need bodies. There are a LOT of them... so I think the hook function bodies should be done in separate patches. Sigh.
This is about to become a meta-issue...
Anyway, this patch cleans up the doc in field.api.php (plus one place in field_storage.module where a comment mentioned the wrong hook name), and is in addition to the patch in #8 for field_ui.api.php. webchick said it was OK to go with separate patches...
Sorry it's so big. The doc was a mess. Sigh.
Comment #12
jhodgdonA note on the "forbid" hooks: It looks like the name of the hook was changed from hook_field_update_field_forbid() to hook_field_update_forbid() at some point. So there were two function docs for that hook. I removed one.
Comment #13
sunDouble space here. (please use diff -up for rolling patches)
There was a time we wrote "id" upper-case everywhere, but at some point we decided that "ID" is not correct. Please don't ask me when or where, but since that happened, most new docs use "id", when referring to some "xyz id".
When describing array keys and values, we do not wrap the key/value (before the colon/description) in quotes. At least, in all new docs.
The usage of "(default)" squeezed behind the value looks wrong to me. I'm not sure whether we use "(default)" anywhere else, but if we want to keep using it here, then it should be placed like "(optional)" behind the colon, before the description.
wow, as I've seen this in another patch just recently -- is this really proper English grammar? I've never seen this anywhere else...
Especially here, the sentence turns into a punctuation flood; x.x., so replacing with "for example" might be a better idea.
Powered by Dreditor.
Comment #14
jhodgdon- Sorry about the diff -up. I use eclipse to make patches on this machine and it doesn't have that option that I've been able to find.
- id vs. ID - my dictionary definitely says ID means identifier/identification, and id is a psychological term (ego, supergo, and id). I believe that is standard English usage. Our doc standards say to use standard English punctuation and usage, and I haven't seen us say to use "id" rather than "ID" in any of our style manuals. If "id" has become the standard, I think it's wrong.
- key in quotes: we're not consistent, but I thought it was merited here because the keys have spaces in them.
- (default) is used all over the place in other doc, but again it's not consistent where it's placed.... I'd be happy if we agreed upon a standard and followed it, and I'm not sure what the standard should be. What are you proposing? I'm happy to adopt anything reasonable as the standard.
- "e.g." means "for example. It should always be followed by a comma. Here's one reference (the bottom example shows the comma in action): http://www.grammarbook.com/punctuation/commas.asp -- I'm happy if it gets replaced by "for example" though.
Comment #15
jhodgdonOn the id vs. ID thing: http://dictionary.reference.com/browse/id -- they list ID or I.D. as possibilities for identification, and "id" as "the part of the psyche, residing in the unconscious, ..."
I checked several other on-line dictionaries as well -- they all agree.
Comment #16
sunwow. Love that punctuation reference. Just learned a lot. :)
So it seems that my issues on "ID" and "e.g." are moot... if we want to be super-nitpicky, then we might want to open a separate issue to fix all docs throughout core regarding them.
Regarding quotes for key/value in lists: Most recent patches changing or adding documentation code removed or didn't contain them. For me, they'd only make sense if the key/value would contain a colon; but as of now, this case does not exist anywhere in core and contrib. Additionally, the API's HTML output is easier to read for me (less punctuation, so visually easier to parse).
Regarding (default), the only reasonable thing we can do is to follow our existing syntax for (optional); i.e., write it after the key + colon, but before the description. btw, totally separate issue: I always wondered whether there is an industry standard for "(optional)" in parameter descriptions. I like our usage, but the amount of developers who need to get used to it, likely means that it's a Drupal-only standard.
Comment #17
jhodgdonOK, it sounds like we have conventions and agreement:
a) Don't put array keys in quotes, when used as lists unless they contain colons (which is unlikely anyway).
b) For default/optional in lists, do:
- key: (optional) The optional something.
- Key: (default) The default something.
c) Follow standard English usage on ID (not id) for identification
d) Follow standard English punctuation on putting commas after e.g. and i.e.
I'll put (a) and (b) into http://drupal.org/node/1354 (doxygen doc) and fix the patch in #11 accordingly, but probably not until Monday. (c) belongs in http://drupal.org/node/338208 if anywhere, but since both c and d are standard English, I really don't think they need to be mentioned anywhere. Maybe (c) because it's been done wrong all over the place...
There's still a separate patch on #8 that needs review, by the way. #13 is needs work.
Comment #18
jhodgdonI just took care of putting (a) and (b) in the list section of node 1354 and added (c) to the style guide.
Comment #19
sunoh, sorry, I thought that #13 would contain #8. #8 is RTBC then.
Comment #20
jhodgdonHere's a replacement patch for #11. Changes (to address sun's comments in #13):
- removed double-spaces after periods at end of sentences.
- removed '' around array keys in lists
- replaced e.g. with for example in several spots
- made sure (default) comes after : in lists
To review: sun has marked #8 patch as RTBC. This patch has not yet been reviewed.
Comment #21
HedgeMage CreditAttribution: HedgeMage commentedRTBC with one tiny minor gotcha...
On line 185 of the patch (line 317 of the patched field.api.php), "formatters' " is changed to "formatter's" when it should really be "formatters' " (because the previous line indicates that there may be more than one formatter).
Comment #22
webchickYEAH!! Great job on this!!
Made that one adjustment and committed to HEAD! :D
Comment #23
jhodgdonOK... The patch in #8 is separate and RTBC and I don't think it was committed.
Also, you may have noticed all the @todo lines in field.api.php -- a lot of those hook doc functions are still lacking documentation, so this issue is not quite fixed. :) So after #8 is committed, please set back to "active" so we can add function bodies to the hooks. Thanks!
Comment #24
Dries CreditAttribution: Dries commentedCommitted #8 to CVS HEAD.
Comment #25
jhodgdonPutting back to "active" so we can remember we still need to add function bodies to all the hooks in field.api.php. They should all have @todo lines.
Comment #26
jhodgdonOK. I'm working on a patch for this now... should be done today or tomorrow.
Comment #27
jhodgdonHere's a patch that adds function bodies to many of the hooks in field.api.php. Most of these were copied directly from implementations in taxonomy.module, field_ui.module, field_sql_storage.module, etc. A few were adapted slightly from implementations in field test modules.
I didn't do any editing on the ones that were copied from core implementations, but maybe some of them should be edited. Thoughts?
Oh yeah, and I removed the body for hook_field_attach_form() because it was totally wrong (was assuming totally different arguments).
Still missing:
hook_field_attach_delete
hook_field_attach_delete_revision
hook_field_attach_form
hook_field_attach_insert
hook_field_attach_load
hook_field_attach_preprocess_alter
hook_field_attach_presave
hook_field_attach_submit
hook_field_attach_update
hook_field_attach_validate
hook_field_create_field
hook_field_create_instance
hook_field_delete_field
hook_field_delete_instance
hook_field_update_instance
hook_field_storage_pre_load
hook_field_storage_pre_query
hook_field_read_field
hook_field_read_instance
All of these missing hook bodies are for hooks added "just in case" or for API consistency, which currently have no application in core. Any contrib modules we can look at for implementation examples?
Comment #28
joachim CreditAttribution: joachim commented@#1 -- we have a hook for field formatter settings but we're not using it? O_o
Comment #29
jhodgdonThis hook was half-way imagined but it was never invoked, so I wouldn't say we "have" one.
Comment #30
joachim CreditAttribution: joachim commentedIs there an issue for this? I couldn't find anything on the hook name. Lack of formatter options was a major stumbling block in D6 and led to TonOfFormatters syndrome. It would be a real shame to have the same problem on D7 for just a lack of a UI.
Comment #31
aspilicious CreditAttribution: aspilicious commentedQuick style review
I think this line exceeds 80 characters...
Powered by Dreditor.
Comment #32
jhodgdonRE #31: It is allowed for in-code comments to exceed 80 characters. The 80-character line limit is for doxygen headers only.
RE #30: I have no idea. See comment #1 above - yched may know. My concern is that we don't document a hook that doesn't ever get invoked.
Comment #33
yched CreditAttribution: yched commentedre #30 - UI for formatter settings : hm, actually there's no dedicated existing issue that I know of.
#553298: Redesign the 'Manage Display' screen is the current issue for the 'display fields' page, but focuses on a different aspect - see comment #104 over there.
Comment #34
sunum, no? The 80 characters limit per line for comments applies to all code comments. Somehow, this seems to have gotten lost on the coding standards pages over time. :-7
Comment #35
dhthwy CreditAttribution: dhthwy commentedIt's per line or 80 character limit per the comment? Is that with the // or minus that?
"// Iterate through the fieldable entities again to attach the loaded term data." is actually only 79 characters. And "Iterate through the fieldable entities again to attach the loaded term data." is 76 characters. If the 80 character limit is for the entire line including whitespace then worst case scenario is (depending on the IF block level you're commenting on) you wouldn't have any room for comments at all. Of course you shouldn't have that many levels of IFs, but point stated.
Comment #36
jhodgdon80 character limits include whitespace in general.
I think the reasons in #35 are why the coding standard for in-code comments is the same as for code -- we allow wrapping past 80 characters. I think?
Comment #37
jhodgdon#27: 675116-somebodies.patch queued for re-testing.
Comment #38
aspilicious CreditAttribution: aspilicious commentedStill green but I think it's best to wait until #780154: There is no listing API for field API gets in.
This one will need a reroll afterwards.
Comment #39
jhodgdonOr that one could wait until this one gets in. That other patch has some style issues (as I just noted there) and needs editing anyway.
Comment #40
aspilicious CreditAttribution: aspilicious commentedYeah but that patch changes a function header, so the example needs to be rewritten.
(or maybe I just overlooked it)
Comment #41
jhodgdonHopefully that patch has a new function header in it for whatever it changed.
Comment #42
jhodgdonOK... that issue has gotten in.
So the patch above doesn't appear to apply at all. Needs a complete reroll. If someone wants to take it on, feel free. I may or may not get to it later in the week.
Comment #43
jhodgdon#27: 675116-somebodies.patch queued for re-testing.
Comment #45
jhodgdonHere is a reroll of the patch in #27, with fix for #31. See comments there.
NOTE: If this patch is accepted, please set status back to "active", since there are still hooks missing function bodies after this patch. This patch just does some of them (the easy ones).
Comment #46
jhodgdon#45: 675116-45.patch queued for re-testing.
Comment #47
bleen CreditAttribution: bleen commentedI've been looking at all 24k of this patch for about 30 mins now and everything looks really good. I think someone with some more field api experience still needs to give his/her blessing ... but from my point of view this is a great chunk of much-needed documentation
Comment #48
yched CreditAttribution: yched commentedThis looks great, thanks a ton, jhodgdon.
Comment #49
Dries CreditAttribution: Dries commentedI agree that this looks good. Committed to CVS HEAD.
Comment #50
bleen CreditAttribution: bleen commented#45 asked this stay active so we can add even more docs via this issue
Comment #51
andyposthttp://drupal.org/cvs?commit=397310 also commited some taxonomy and theme related hunks
Comment #52
jhodgdonStatus:
- field_ui.api.php looks OK to me now.
- In modules/field/field.api.php there are still some hooks missing function bodies:
hook_field_attach_form
hook_field_attach_load
hook_field_attach_validate
hook_field_attach_submit
hook_field_attach_presave
hook_field_attach_insert
hook_field_attach_update
hook_field_attach_preprocess_alter
hook_field_attach_delete
hook_field_attach_delete_revision
hook_field_storage_pre_load
hook_field_create_field
hook_field_create_instance
hook_field_delete_field
hook_field_update_instance
hook_field_delete_instance
hook_field_read_field
hook_field_read_instance
They all have @todo lines in there as the function body, so it's pretty easy to find them in the file... the harder part will be writing sample function bodies for them. Does anyone know of a place in core/contrib that would have examples?
Comment #53
Damien Tournoud CreditAttribution: Damien Tournoud commentedDuplicating the body of core functions in the example API is a really bad idea. Please stop immediately.
We currently have *the whole* field SQL storage module duplicated in field.api.php. We don't need the duplicate maintenance burden of this. If a core function is the best example for the implementation of a callback of hook, just add a link to there.
Comment #54
Damien Tournoud CreditAttribution: Damien Tournoud commentedAt the very minimum, we need to remove the Field SQL Storage from here. It's a complex beast.
Comment #55
tstoecklerCould we use '@see' instead of 'See' to make the functions clickable on api.drupal.org? That way, I think the user 'overhead' (for not having the 'example' code right there) would be minimal.
Comment #56
Damien Tournoud CreditAttribution: Damien Tournoud commented@tstoeckler: I think the API document makes any reference to comments clickable... but maybe that only works in the docblock.
Comment #57
jhodgdonAny function name followed by () within a docblock /** */ is clickable within api.drupal.org. Any function call in a function body is clickable too. Nothing in code comments // within function bodies is clickable.
So. That aside, the philosophy of the *.api.php hook body functions is to provide, **on that page on api.drupal.org** an example of how your module could implement the hook. If you would like to write different examples, or simplify the functions taken from core down to where they would be better examples, that would be lovely. But just linking to the existing implementation in core is not how the rest of the hook_* documentation is done, and not what we want to do here. So please don't do that. OK?
By the way, I am on a cycling holiday right now on the Danube river, and will be out for the next six weeks, checking email and/or issue queues occasionally, so I'm not up to writing any patches... If someone else wants to write these function bodies, that would be great. But just linking to a different function is not OK.
Comment #58
Damien Tournoud CreditAttribution: Damien Tournoud commentedGod invented hyperlinks, probably for a reason :)
Comment #59
jhodgdonSorry..... We need function bodies for hooks. Real function bodies. That is how we document hooks, until we change the standards, which is a whole different discussion, which you could open as a separate issue. But until and unless that is changed, we need function bodies. Period.
Comment #60
dmitrig01 CreditAttribution: dmitrig01 commentedBump. To quote Damien above, "We currently have *the whole* field SQL storage module duplicated in field.api.php. We don't need the duplicate maintenance burden of this. If a core function is the best example for the implementation of a callback of hook, just add a link to there."
Comment #61
gddI agree with jhodgdon that these should stay as they are for D7. Changing them just for the field API hook implementations would be confusing and irritating, and changing them drupal-wide is a discussion that should happen in the D8 cycle.
Comment #62
jhodgdonIt is definitely too late to change how we document hooks Drupal-wide.
We also don't have a really good way of putting a link inside a function body that will show up as a link on api.drupal.org (see #57), and our current doc standards say that we need sample function bodies in all hook documentation blocks.
So. We need some function bodies. Unless someone wants to step up and write sample functions, let's get this in... any seconds?
Comment #63
webchickAgreed that we're not changing the way hook docs are done now. Sounds like a nice patch for some combination of API module/core in Drupal 8, though.
Let's just get 'er done. AFAIK this is the last of the issues we need to close to be at 100% hook documentation compliance.
My only other suggestion would be to see if Examples module has a simpler example that wouldn't duplicate quite as much code. But if not, copy/paste from core and let's close this out.
Comment #64
jhodgdonAs per #63, please ignore Damien's patch in #54 that removes the function bodies for the hooks that had them.
So what we need now is function bodies for the hooks still missing bodies, which are listed in #52... let's see, I'll double check the list. None of these have examples in core that I can tell...
Yes, these hooks are still missing function bodies (all in modules/field/field.api.php):
hook_field_attach_form
hook_field_attach_load
hook_field_attach_validate
hook_field_attach_submit
hook_field_attach_presave
hook_field_attach_insert
hook_field_attach_update
hook_field_attach_preprocess_alter
hook_field_attach_delete
hook_field_attach_delete_revision
hook_field_storage_pre_load
hook_field_create_field
hook_field_create_instance
hook_field_delete_field
hook_field_update_instance
hook_field_delete_instance
hook_field_read_field
hook_field_read_instance
Setting back to "active" since we have no patch for these hooks at this time.
Comment #65
jhodgdonComment #66
jhodgdonI've proposed this as a GCI task here:
#948752: Create hook function bodies for Field API hooks
Comment #67
EvanDonovan CreditAttribution: EvanDonovan commentedCould we just copy and paste them from core then as webchick suggested in #63? I think Damien's idea made more sense, but if there's no way to create a hyperlink from a docblock, then I guess it's no go for this release.
Comment #68
jhodgdonThese particular hooks have no examples in core, as far as I know (last time I checked anyway).
The idea from Damien above was about the last set that went in, which were (slightly simplified in some cases) examples from core. He wanted to delete them.
Comment #69
EvanDonovan CreditAttribution: EvanDonovan commentedI thought that Damien was saying that field_sql_storage was where core implemented them. Sorry if I misunderstood.
Comment #70
jhodgdonRight. field_sql_storage module implemented the hooks added as examples in the patch in #45, which was committed. Damien was trying to get that patch rolled back in his patch in #54. It's confusing. You need to read the entire thread starting after #45 to understand the discussion.
Comment #71
EvanDonovan CreditAttribution: EvanDonovan commentedOh, I understand now. I knew that Damien wanted to remove from api.php & cross-reference the core implementations, but I didn't realize that the hooks from #64 had no example implementations in core. Sorry for creating confusion in my efforts to help...
Comment #72
jhodgdonThis issue has been posted as a Google Code-In task.
http://www.google-melange.com/gci/task/show/google/gci2010/drupal/t12904...
Comment #73
rschwab CreditAttribution: rschwab commentedSubscribination!
Comment #74
catchMoving to a task. Not clear if this was fixed in other issues?
Comment #75
jhodgdonThis is a documentation bug. I'm willing to have it not marked "major", and I understand that we don't want to hold up progress based on this issue not being fixed, but let's leave it as a bug. (It's a bug that this documentation is missing that should be there.)
It looks like the list in #64 is still correct. I may not have checked all of them, but at least most of them still have
// @todo Needs function body.
as their function body.
Comment #76
jhodgdonThese hooks do not exist in 8 any more. Most of them still need function bodies in 7.