Clean up API docs for field module

Do you plan to patch this module? If so can you assign the issue to yourself and also add your name to the summary issue? Thanks!

The beginning of the file field.module is a long defgroup with lists. The lists don't follow the Doxygen list format: they have extra newlines and indentation. This makes it easier to read in the code and doesn't seem to change how it shows on the API site. Should I leave the extra indentation and newlines in?

Actually, the way it is shown on the API site isn't good, because the lists are lacking in the usual : punctuation -- see
http://api.drupal.org/api/drupal/core--modules--field--field.module/grou...

IMO they should be fixed.

I was planning to fix everything about the lists except possibly for the indentation and newlines, since these make it easier to read in the code and don't seem to affect how it appears in the API pages. What do you think about this?

I think the indentation and newlines should be fixed too. Why should we leave the lists using non-standard formatting, if you are going to be editing them anyway?

I was about halfway through the process of removing the indentation and newlines when I realized how much more difficult to read this made the text in the source files. I wanted to confirm that I was not doing something that would turn out to be a mistake. Thanks for confirming.

A few questions so far...

In field.form.inc
- field_default_form() has a parameter $get_delta that I'm not sure how to explain. Any ideas?
- field_add_more_submit() is the submission handler for the "Add another item" button of a field form. I'm not sure how much I can or need to make this conform to the usual form function documentation.

In field.default.inc, we have field_default_validate() and field_default_submit(), which are validation and submit handlers for individual fields. I'm not sure how much I can or need to make these conform to the usual form function documentation.

Thanks for your help...

RE #8 - $get_delta... I'm not sure why it's named exactly that way, but it looks like it allows you to pass in a value for "delta", which is the index for a specific value in an array of field values.

RE #8 - submit handlers for buttons - what we've been doing on other issues is something like this:

Form submission handler for the xyz button on form_function_name().

You should be able to do something like that for the field submits too?

First pass on this very long module.

Probably not applying on account of #334024: Use const keyword to define constants instead of define().

Also: Wow, that one is even bigger than my includes patch! Awesome work. :)

Thanks, that was confusing. I think I have fixed the patch.

+++ b/core/modules/field/field.form.inc
@@ -6,7 +6,29 @@
+ * @param $entity
+ *   The entity the field belongs to.

This needs to state that the $entity can sometimes be absent, given the following takes place further down in the function:

  // This could be called with no entity, as when a UI module creates a
  // dummy form to set default values.
  if ($entity) {
    list($id, ,  ) = entity_extract_ids($entity_type, $entity);
  }

I *assume* that 'absent' means it's set to FALSE, though there's no concrete indication of that...

There is no default supplied for $entity in the function signature, so it will throw a fatal of nothing is passed. I guess we could check callers to see if they're passing FALSE?

tagging.

Also @xenophyle: are you still planning to work on this? If so, please respond. If not, we'll unassign it so someone else can. Thanks!

This no longer applies and needs rerolled :/

I tried to apply the most recent patch and it failed. It appears that the patch must be re-rolled manually.

assigning to self so I can work on re-rolling this MASSIVE patch. :)

Whew. I think I got it.

Hopefully this gets approved and rolled in soon so we don't have to redo it. Since it shouldn't affect ANY functionality (strictly documentation of methods), it should be good to go.

self caught a typo. I missed a space; here's the corrected one. :)

Pushing back to needs work due to a tyoe in a documentation string, dellintosh is aware of the issue and re-rolling -- just marking so no one pushes this before it's baked.

wow... speedy dellintosh is speedy.

Docs are all good; +1 to commit this from me.

Applies cleanly.

Robin: did you actually review the entire patch, or just verify that it applies (I can tell that from the test bot)? We need someone to review the content of the patch and make sure that the doc is updated correctly.

Reviewed the entire patch (hence how I caught the space that required a re-roll 3/4 of the way though), looking for grammatical issues, issues with terminology and checked any new param entries against the functions to ensure they looked sane and had no obvious issues.

/Robin

Thanks @dellintosh and @Robin Monks! Huge patch, lots of work. :) Nice job on the reroll.

I reviewed starting in options.api.php through the end of the patch (ran out of time today) and found some small things to fix:

1. +++ b/core/modules/field/modules/options/options.moduleundefined
   @@ -2,7 +2,9 @@
   + * Defines select, checkbox and radio button widgets.
   

   There should be a comma here after "checkbox".

2. +++ b/core/modules/field/modules/options/options.moduleundefined
   @@ -163,7 +166,7 @@ function options_field_widget_settings_form($field, $instance) {
   + * Form validation handler for options elements.
   

   I think it should actually be "Form element validation handler for..."?

3. +++ b/core/modules/field/modules/options/options.moduleundefined
   @@ -236,6 +251,16 @@ function _options_properties($type, $multiple, $required, $has_value) {
   + * @param $field
   + *   The field structure.
   + * @param $instance
   + *   The instance structure.
   + * @param $properties
   + *   An array containing the properties of the widget.
   

   Maybe we can add @see to somewhere where the details of these structures are defined? (This applies to the added param docs for several of these functions).

4. +++ b/core/modules/field/modules/options/options.moduleundefined
   @@ -236,6 +251,16 @@ function _options_properties($type, $multiple, $required, $has_value) {
   + * @return
   + *  The option array for the field.
   

   The indentation is off here (short a space on the description).

5. +++ b/core/modules/field/modules/options/options.moduleundefined
   @@ -260,6 +285,11 @@ function _options_get_options($field, $instance, $properties) {
   + * @param $options
   + *   The option array.
   
   @@ -280,6 +310,18 @@ function _options_prepare_options(&$options, $properties) {
   + * @param $options
   + *   The option array.
   

   What is "the option array"? Can we add more detail here? Or maybe an @see to where "the option array" is defined? (Also, I think it is an "options array" rather than an "option array," correct?)

6. +++ b/core/modules/field/modules/options/options.moduleundefined
   @@ -280,6 +310,18 @@ function _options_prepare_options(&$options, $properties) {
   + * @param $column
   + *   The field storage columns of the field.
   ...
    function _options_storage_to_form($items, $options, $column, $properties) {
   

   Can we add more detail here? Also, the parameter name seems to be singular, but the description is plural, which seems contradictory. I think it is just a string with one column name?

7. +++ b/core/modules/field/modules/options/options.moduleundefined
   @@ -327,13 +375,13 @@ function _options_form_to_storage($element) {
     * @param $array
   - *   The array to be transposed. It must be at least two-dimensional, and
   - *   the subarrays must all have the same keys or behavior is undefined.
   + *   The array to be transposed. It must be at least two-dimensional, and the
   + *   subarrays must all have the same keys or behavior is undefined.
     * @return
     *   The transposed array.
   

   Looks like we need a blank line between the @param and @return here.

8. +++ b/core/modules/field/modules/text/text.moduleundefined
   @@ -144,9 +144,9 @@ function text_field_validate($entity_type, $entity, $field, $instance, $langcode
   + * Where possible, thie function generates the sanitized version of each field
   + * early so that it is cached in the field cache. This avoids the need to look
   + * up the field in the filter cache separately.
   

   Typo: "thie"

Anyone is welcome to reroll with these fixes now, or to wait for the rest of a full review. In either case, please include an interdiff to facilitate easier reviews. Thanks!

Oh also, we should double-check that #14/#15 have been addressed.

assigning back to myself...

attaching interdiff?

forgot the patch too... here's both. :)

Thanks! NR for the bot.

also, for those who prefer a massive patch (with both combined...)

I haven't fully reviewed it, but what I did read looks good and, except for a couple places where I might have stuck a comma (i.e. trivial), I wouldn't change anything I can see... but of course I'm no expert (on the technical aspects of this module), so I'll be referring to this... a lot. Great work!

LoMo: If you could point out those places that need a comma, that would be helpful. :)

Okay... sometime in the next couple of days I'll put on my "proofreading hat" and mark it up. I'm sure the learning process I'll get out of carefully reading all of this, not to mention the (trivial) community contribution of a few grammatical corrections, will make this a worthwhile task for me. :-)

@LoMo -- Sounds great. :) You might want to check out dreditor, which provides a handy way of adding comments about specific code hunks in patches.

- Comments #14/15 above have not been taken care of in the current patch.
- It looks like items 3 & 5 & 6 in comment #28 were not addressed either.

For item 5 in comment #28, "option array" seems preferable to "options array" following the usual convention of using the singular as an adjective: "coat rack", not "coats rack", and "shoe box", not "shoes box". A compromise might be "An array of options."

Thanks to everyone for picking up my dropped doc cleanup issues (my workload increased greatly in the new year). You have amazing stamina.

Thanks @xenophyle! Yeah, I'd really like a better explanation of what it is, since "option(s) array" isn't a thing I know about. "Array of options" is at least more clear, but I'd still want more detail on what an "option" is (i.e. the parameter structure).

Right, it's never been completely clear to me how much technical detail is needed for this cleanup sprint. It can be overwhelming to understand the module well enough to write really good descriptions.

Although not usually a fan of doing this; I'd recommend apply this massive list of changes with a broad brush and coming back with updates to single or related functions in batches later. Otherwise with the moving target that this patch needs to apply against it will take much longer to materialize.

@xjm: Thank you for telling me about that. I hadn't even used Greasemonkey scripts before, so there was a small learning curve. I've checked it out and it's very nice, although I'm still not sure about how one works with "numbering sections" for discussion. I'll have to play with it some more.

But now, looking at the patch, I'm 1) overwhelmed trying to make sense of this massive patch outside the context of all the rest of the code/documentation and 2) really just seeing very trivial things that could either wait to go in at a later time or which might be too trivial to mess with.

I agree with #45. The patch should be committed. That way it will be simpler to continue with any further edits needed (i.e. new patches, if necessary). For now, I will gracefully back out of my suggestion to proofread the patch, but I think that dreditor will be useful when reviewing other patches, especially once I've really figured out how it's supposed to work (beyond the basics, which I've seen in "review mode").

Those are all valid points. The disadvantages of postponing corrections are:

• Some of the burden is shifted from patch authors, true, but it is shifted to reviewers and committers, who are in shorter supply.
• Cleaning up lines once is less disruptive to other, functional changes than cleaning them up twice. (This is the most important point, to me--we need to minimize the extent to which we disrupt actual bugfixes and work on D8.)
• There is a legitimate concern that the followups don't actually happen. We saw this with the node module cleanup: when it came time to backport that, it became clear that a lot of things were missed when I myself said "we should just commit it already" as others have here. :)

On the other hand I agree that these 200k monsters are exhausting for everyone involved, especially for those who may not have much experience resolving merge conflicts. If anything, I'd prefer perhaps multiple, thorough, correct patches... one per file, perhaps. I've also experimented with rolling my cleanup patches as a series of interdiffs (one per task).

Ultimately, though, I believe this is up to jhodgdon's discretion both as the API documentation maintainer and as the cleanup committer.

Thanks everyone for your continued efforts on this issue!

Oh, I forgot to mention--these patches generally cannot be committed anyway when we are over issue thresholds, so we might as well continue to refine them since we'll likely need to reroll them regardless.

Here is a reroll of the patch with additional changes. Interdiff from #34.

Additionally,
I think field_test_entity_add() and field_test_entity_edit() need to be updated per http://drupal.org/node/1354#menu-callback

field_test_entity_form_validate(), field_test_entity_form_submit(), field_test_entity_nested_form(), field_test_entity_form_submit_build_test_entity(), and almost every other form in field_test.entity.inc should be updated per http://drupal.org/node/1354#forms

template_preprocess_field(), and template_process_field(), need to be updated per http://drupal.org/node/1354#hookimpl

I just don't know enough about this module to correct those issues.

Hopefully this one will work. I'm not sure what happened with the last one. Interdiff from #34 again.

Corrects the two whitespace issues in the previous version.

dellintosh,

Sorry, I didn't mean to assign this to myself. Do you still plan on working on this?

No worries... just hoping to get this merged in soon to avoid having to redo it later (which since this took several hours to complete is not something I'd be looking forward to).

dellintosh,

Can you fix the issues I identified in #49

Status update (see #56 and #49).

Hello NROTC_Webmaster

Looks like the Test error in #49 was internal error, because the test past at #51 and #52

Thanks @-enzo-! However, it sounds to me like @NROTC_Webmaster's issue with #49 was not the test failure, but rather some points on which he'd like assistance:

Additionally,
I think field_test_entity_add() and field_test_entity_edit() need to be updated per http://drupal.org/node/1354#menu-callback

field_test_entity_form_validate(), field_test_entity_form_submit(), field_test_entity_nested_form(), field_test_entity_form_submit_build_test_entity(), and almost every other form in field_test.entity.inc should be updated per http://drupal.org/node/1354#forms

template_preprocess_field(), and template_process_field(), need to be updated per http://drupal.org/node/1354#hookimpl

I just don't know enough about this module to correct those issues.

I'll try to help out with these. Also keeping an eye on #1785256: Widgets as Plugins. We should make sure not to collide with that patch.

So yeah, 33 merge conflicts after #1785256: Widgets as Plugins. I missed before that it was from April.

On the upside, the widget-as-plugin changes include vastly superior documentation.

In the spirit of #1310084-98: [meta] API documentation cleanup sprint, here's a git apply --reject of #52.

My vote is to commit these cleanups once reviewed (and I believe they've already been reviewed), and then give it another pass for the changes that this leaves out.

+++ b/core/modules/field/field.api.phpundefined
@@ -87,7 +87,7 @@ function hook_field_extra_fields_alter(&$info) {
- * Define field types.
+ * Define field types, widget types, display formatter types, storage types.

I think this probably needs an "and".

+++ b/core/modules/field/field.api.phpundefined
@@ -1166,7 +1174,10 @@ function hook_field_attach_submit($entity_type, $entity, $form, &$form_state) {
+ *   The entity with fields to process

@@ -1177,7 +1188,10 @@ function hook_field_attach_presave($entity_type, $entity) {
+ *   The entity with fields to process

@@ -1188,7 +1202,10 @@ function hook_field_attach_insert($entity_type, $entity) {
+ *   The entity with fields to process

@@ -1218,7 +1235,10 @@ function hook_field_attach_preprocess_alter(&$variables, $context) {
+ *   The entity with fields to process

@@ -1229,7 +1249,10 @@ function hook_field_attach_delete($entity_type, $entity) {
+ *   The entity with fields to process

@@ -1369,7 +1392,10 @@ function hook_field_available_languages_alter(&$langcodes, $context) {
+ *   The entity with fields to process

These need a period.

+++ b/core/modules/field/field.moduleundefined
@@ -257,14 +250,14 @@ const FIELD_BEHAVIOR_DEFAULT = 0x0002;
 /**
- * Age argument for loading the most recent version of an entity's
- * field data with field_attach_load().
+ * Age argument for loading the most recent version of an entity's field data
+ * with field_attach_load().
  */
 const FIELD_LOAD_CURRENT = 'FIELD_LOAD_CURRENT';
 
 /**
- * Age argument for loading the version of an entity's field data
- * specified in the entity with field_attach_load().
+ * Age argument for loading the version of an entity's field data specified in
+ * the entity with field_attach_load().
  */
 const FIELD_LOAD_REVISION = 'FIELD_LOAD_REVISION';

These should probably be reworded to be one line rather than just rewrapped.

I'll reroll to fix these things, and then I'll probably set it to RTBC. (Not actually my patch.) There are a lot of really good cleanups in there, and I think it's important to get this in as soon as possible so that it doesn't disrupt Field API work in the next two months.

Alright, I think this is RTBC enough. :) I realize I'm partially contradicting what I said in #47, but what's in here has been reviewed several times already, and we can reopen this issue again once the current patch is committed to add further cleanups.

This one's for Jennifer.

I agree that this one has been worked on and reviewed enough, so I committed the patch.

I know that there are more cleanups that can be done. For instance, I took a look at field.attach.inc, and the very first function in there needs to have its first line verb tense fixed:

/**
 * Invoke a method on all the fields of a given entity.

So maybe the next patch on this issue could go through all the files in this project and just look for first-line verb tense problems? Then we can see what else needs to be done.

Thanks @jhodgdon! That sounds like a good idea to me.

These issues are a lot of work with very little tangible payoff, so I'm closing the rest of them as "won't fix". Your efforts on working on this issue were appreciated... it was just my fault for starting a task that was very difficult to get right.

Let's instead put our effort into fixing and reviewing documentation that is really unclear and/or wrong, and I hope that the people who worked on these issues are not afraid to jump into a more reasonable issue!