Support from Acquia helps fund testing for Drupal Acquia logo

Comments

joachim’s picture

Title: hook_field_settings_form should be in Field Types API topic » hook_field_settings_form, hook_field_instance_settings_form should be in Field Types API topic

hook_field_instance_settings_form should be too.

sven.lauer’s picture

I suppose one reason why these are not linked presently (though not necessarily a good one) is that these hooks are provided by field_ui.module, while the other field API hooks are provided by field.module itself. I guess this needs discussion in so far one could also make a case for a "Field UI API".

joachim’s picture

Ah that would explain why they're not in there.

But from a developer's POV, I want to see a group that lists all the hooks I need to implement to make a practical, working field: that starts with the field type hook and then goes on to the various settings forms, and finally ends with the widget form itself.

sven.lauer’s picture

But from a developer's POV, I want to see a group that lists all the hooks I need to implement to make a practical, working field: that starts with the field type hook and then goes on to the various settings forms, and finally ends with the widget form itself.

IFF you are defining a field that is intended to be administered through field UI. If you are writing a module that defines a field that is completely handled through code (perhaps forced to be handled by code by setting no_ui => TRUE), you won't be defining the settings forms.

Not disagreeing that these should be linked in Field Types API, though, just pointing out that there is a difference.

jhodgdon’s picture

I don't see a reason not to list them, and agree with joachim that they are needed by at least some field type developers. How about a patch?

sven.lauer’s picture

Just waited for another opinion on this. Patch attached.

sven.lauer’s picture

Status: Active » Needs review
jhodgdon’s picture

So... This is good, as far as it goes. But I noticed at the top of the file:

/**
 * @ingroup field_ui_field_type
 * @{
 */

This tries to add everything in the file to this topic, but ... the topic does not exist. Also, I don't think @ingroup {} works (should be @addtogroup).

So I think instead of this patch, we should probably add everything in this file to the Field Types API topic, by changing this line (and the closing comment) to the right codes. Shouldn't we?

Also, the existing Field Types API page is kind of ... no it's really really lame.
http://api.drupal.org/api/drupal/core--modules--field--field.api.php/gro...
It has a bunch of @see in it that should be removed, and it needs some grammar cleanup. For consistency, I would say if we are adding functions to that topic, we should add @see lines to the topic, but I would rather see all of those @see lines removed (they are redundant IMO). So can this patch do that too? We can file a separate issue to clean up the grammar/wording of the topic too (or fix it here)...

jhodgdon’s picture

Status: Needs review » Needs work
joachim’s picture

If we're doing clean-up, I would love to see this bit:

> and a variety of other field hooks are called by the Field Attach API to perform field-type-specific actions.

replaced with some sort of list that walks you through the API, sort of like this:

- hook_field_settings_form() allows additional settings for the field as a whole
- hook_field_instance_settings_form() allows additional settings for specific instances of the field
- ... etc

jhodgdon’s picture

Yeah, that would be good. Should we file a separate issue for that?

sven.lauer’s picture

Title: hook_field_settings_form, hook_field_instance_settings_form should be in Field Types API topic » Clean up Field Types API topic and add settings form hooks

I decided to go ahead and have a shot at rewriting the Field Types API (or should that be called Field Type API?) text.

I started to compile a list of field type hooks that get called by the Field Attach API, but this simply leads to a lot of duplication: Those are basically all the hooks that do not pertain to widgets and formatters, except for hook_field_info[_alter]() and hook_field_schema(). So if we want to have a separate list for those (which makes sense), we really should have a separate topic for field type hooks (sans widget- and formatter-specific stuff).
It has always struck me as somewhat odd that widgets and formatters are included / documented in the Field Types API topic. Perhaps we should simply break these into their own groups (or group, something link "Field (Display) handlers" or something?). Though I guess this would make this a D8 only change, which does not help D7 developers ...

Also, if we mention hook_field_info() in the introduction (as we currently do), maybe we should also add a sentence about hook_field_schema()---for that is where datatypes are really specified (I recall that this stumped me the first time I tried to figure out what exactly the columns and data types for a given field were).

If we had a proper field types topic, we could have something like

In the Field API, each field has a type, which determines what kind of data (integer, string, date, etc.) the field can hold, which settings it provides, and so on. The data type(s) accepted by a field are defined in hook_field_schema(), other basic properties of a field are defined in hook_field_info().
The other hooks below are called by the Field Attach API to perform field-type-specific actions.

sven.lauer’s picture

Status: Needs work » Needs review
FileSize
729 bytes

Aaaand the current patch.

jhodgdon’s picture

Um. This is the same patch as before?

I think making separate topics for definition/formatters/widgets makes sense (the three topics should link to each other). I think that could go into D7 as well as D8.

sven.lauer’s picture

Assigned: Unassigned » sven.lauer
Status: Needs review » Needs work

Arg, just for the record, here is the correct patch (I am an idiot).

But setting back to "needs work", as I am going to roll a patch that splits the topic.

It would be nice to get this into D7, but webchick hasn't been a fan of patches that reorganize the documentation ...

sven.lauer’s picture

sven.lauer’s picture

Status: Needs work » Needs review
FileSize
6.58 KB

Here is a patch that splits up the "Field Types API" group into three.

jhodgdon’s picture

Status: Needs review » Needs work

Related issue (not duplicate I think):
#1345654: hook_field_info should have clear link to the field_types topic, and that topic should say which hooks are needed

Wow, sorry, I apparently forgot to review this and it fell down in my issue list (which runs to many many pages). So I took a look at the patch in #17. It looks mostly good! A few little things to clean up (and likely it also needs to be rerolled):

a) I think Field type doesn't need to be capitalized here:

+ * given @link field_types Field type @endlink may be edited using more than one

b) I think the words "to choose" need to be inserted here:

+ * widget. In this case, the Field UI module allows the site builder which
+ * widget to use.

c) Move link down to next line:

+ * Widgets are @link http://api.drupal.org/api/drupal/developer--topics--forms_api_reference.html Form API @endlink

Oh dang, I have to run out... didn't get past that in the patch. sorry!

jhodgdon’s picture

Review of the rest of the patch:

d)

+ * Field API formatter specify how fields are displayed when the entity to

formatter -> formatters

e)

+ * which the field is attached is displayed. Fields of a given
+ * @link field_types Field type @endlink may be displayed using more than one

Field type -> non-capitalized

f)

+ * formatter. In this case, the Field UI module allows the site builder which
+ * formatter to use.

Needs "to choose" in there.

g) Back up towards the top of the patch, I just noticed this:

+ * and so on. The data type(s) accepted by a field are defined in
+ * hook_field_schema(), other basic properties of a field are defined in
+ * hook_field_info(). The other hooks below are called by the Field Attach API

In that second line, the comma should be a semi-colon (or else put "and" in there).

And did I mention -- Sven, you did a really nice job on this documentation. :) Maybe though in the Widgets and Formatters sections, we should put a line or two kind of like this part of the Field Types section:

+ * In the Field API, each field has a type, which determines what kind of data
+ * (integer, string, date, etc.) the field can hold, which settings it provides,
+ * and so on. The data type(s) accepted by a field are defined in
+ * hook_field_schema(), other basic properties of a field are defined in
+ * hook_field_info(). The other hooks below are called by the Field Attach API
+ * to perform field-type-specific actions.

Just to give a heads up, like "This is the main hook"?

sven.lauer’s picture

Here is a patch that fixes the issues in #19 and #20. I also added a sentence pointing to the _info hook to the widget and formatter topics.

Thanks for the kind words! And, as usual, thanks for being an awesome doc maintainer!

sven.lauer’s picture

Status: Needs work » Needs review
jhodgdon’s picture

Dang, this fell off my list again... Let's retest, reroll if necessary, then get this reviewed and committed.

jhodgdon’s picture

Status: Needs review » Needs work

The last submitted patch, doc-add-settings-hooks-to-field-type-group-1391118-4.patch, failed testing.

jhodgdon’s picture

Status: Needs work » Needs review
jhodgdon’s picture

Status: Needs review » Fixed

Great! Committed to 8.x and 7.x.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.