hook_field_attach_purge [see function field_purge_data() in modules/field/field.crud.inc]

This is part of this meta-issue: #675046: Make sure all hooks in D7 have documentation

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

robeano’s picture

Status: Active » Needs review
FileSize
2.09 KB

As usual, I dislike my example, but it's ready for review.

jhodgdon’s picture

Status: Needs review » Needs work

a)

This hook is invoked in field_purge_data and allows ...

You need to put () after function names, so the API module can turn them into links on api.drupal.org.

Also maybe in the first line -- is field_attach_purge a function name? If it isn't a function name, perhaps remove the _ characters?

b) We try to put @see at the end of the docblock. That See @link line can probably go there too.

c) The example -- maybe it should also check whether the field matches, besides just that it's node data? Though doing that might be complicated....

puregin’s picture

FileSize
13.51 KB

The attached patch does the following:

1. puts () after the function name field_purge_data()

2. moves the @see block and @link line to the bottom of the docblock.

3. Makes the initial description of each hook consistent: "Acts when... " followed by a description from the
one-line description of the invoking function. This avoids the ambiguity of "Act on X" (i.e., does it act when
X occurs, or does it perform an action on X).

puregin’s picture

Status: Needs work » Active
puregin’s picture

Status: Active » Needs review

Gah. I think I have status right now...

aspilicious’s picture

Status: Needs work » Needs review
+++ field.api.php	21 Apr 2010 07:45:20 -0000
@@ -900,7 +900,9 @@ function hook_field_formatter_view($enti
+ * Acts when adding form elements to a form structure, for all fields for an entity.

If I understand dreditor, this sentence exceeds 80 characters, that isn't allowed.
This is a hook, so acts have to be act.

http://drupal.org/node/1354#hooks

+++ field.api.php	21 Apr 2010 07:45:20 -0000
@@ -1013,7 +1015,7 @@ function hook_field_attach_update($entit
- * Act on field_attach_preprocess.
+ * Acts when populating the template variables with the field values available for rendering.

Same as first one + same thing about the acts

+++ field.api.php	21 Apr 2010 07:45:20 -0000
@@ -1040,7 +1042,7 @@ function hook_field_attach_delete($entit
+ * Acts when deleting field data for a single revision of an existing entity. ¶

Trailing whitespace

I don't know who made dreditor script but he/she made me happy :D. It would be awsome if there was an optimised script for chrome to ;). I can install it on chrome but it doesn't do &!@#

98 critical left. Go review some!

aspilicious’s picture

Status: Needs review » Needs work
puregin’s picture

@aspilious - thanks, dreditor sounds cool, I will check it out.

I've fixed the longer comment lines that I added/modified.

I notice that hooks currently don't seem to follow a clearly established convention for summary line documentation. I'm looking for example at the JavaDocs standard:

Use 3rd person (descriptive) not 2nd person (prescriptive).
The description is in 3rd person declarative rather than 2nd person imperative.
Gets the label. (preferred)
Get the label. (avoid)

I prefer this, but I see examples of both conventions in our documentation (i.e., both 3rd person declarative and 2nd person imperative)

I think I've got the diff current, it the correct (root) directory now. Sorry about the flailing.

puregin’s picture

FileSize
13.83 KB
jhodgdon’s picture

Please don't make patches that go beyond the scope of the issue. If you feel that a lot of things need changing, either file one big issue saying "this needs changing" or a bunch of little ones. And please search the issue queue to see if the issue has been reported first.

So all of the patches from #3 on are not relevant to this issue. robeano is at this moment going back to the patch in #1 and fixing it to address the relevant comments.

Also, as a general courtesy, when an issue is assigned to someone, it is polite to ask them before taking it over for yourself. Thanks!

Also, just looking at your latest patch, some of the changes are good (such as informations -> information), but others are not. For instance,

- * Act on field_attach_form.
+ * Acts when adding form elements to a form structure, for all fields
+ * for an entity.

Please check http://drupal.org/node/1354#hooks
We want this to start with "Act" not "Acts", and the short description should be one line if at all possible.

Thanks!

puregin’s picture

@jhodgdon, thanks for the feedback. I tend to try to fix all instances of a given issue for a particular file at once, but I see your point.

Regarding 'Acts' vs 'Act', I was taking my guidance from #487802: Function doc standard for Drupal is non-standard in industry, which seems to indicate that 3rd person declarative is preferred.

And sorry @robeano if overstepped the bounds of helpfulness :)

robeano’s picture

FileSize
2.14 KB

New patch which follows jhodgdon's advice given in #776694-2: hook_field_attach_purge is not documented

* Added ()
* field_attach_purge was wrong in the one-line description
* Moved the @see to the bottom of the block

puregin, thanks. I was more confused than offended. Ready for another review.

jhodgdon’s picture

puregin: that is not how we do things in the Drupal project. Google "killing kittens" and see what you can find.

And please read the standards page before you comment on whether the doc is following standards. Thanks!

blg@bgreenaway.com’s picture

Status: Needs review » Needs work

Hi robeano, sitting here with jhodgdon at DrupalconSF doc sprint and spotted one thing, suggesting on jhodgdon's advice another.

First, the patch has two additional lines than it needs after '* @see field_purge_data()', one is an empty comment line, another appears to be a blank and empty line. (see below)

+ *
+ * See also @link field_purge Field API bulk data deletion @endlink
+ * @see field_purge_data()
+ *
+
+ */

And we're pretty sure that the line 'See also @link' would benefit from being changed to '@see @link'.
Everything else AOK, plz re-run test (not sure I can at this stage, but will try later if you don't beat me to it:)

aspilicious’s picture

FileSize
2.09 KB

rerroll

aspilicious’s picture

Status: Needs work » Needs review
aspilicious’s picture

Status: Needs review » Reviewed & tested by the community

Fixed the only issue pointed out in #14

jhodgdon’s picture

agreed, looks good!

Dries’s picture

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.