Problem/Motivation

The fieldt-API provides several method to manipulate the data between the form, the formatters and the database (hook_field_presave, hook_field_load, hook_field_formatter_prepare_view, …). Ideally this would lead to a clean separation of up to 4 (possibly different) datastructures of a field item: One in the database, one in the application, one for the widget, one for the formatter.

Sadly this is not the case. Those hooks are not called in any reliable order, because the responsibility for calling the hooks at least partially lies with the entity modules (!).

This circumstance has already lead to bugs and workarounds in modules that implement field types. A good showcase is the taxonomy module, as it provides more than one widget for it's field-type and uses slightly different data-structures for them.

Proposed resolution

I propose to include calls to the corresponding hooks directly in the field-API functions. For backward compatibility a flag may be added that guarantees that each of those hooks is only executed once when it's additionally called by contrib.

Attached is a patch that moves the calls to field_attach_presave() from the entity implementations to field_attach_insert() and field_attach_update().

Remaining tasks

- the flag/check that guarantees that hooks are called only once for backward compatibility has to be added. (ie. use $items['#field_hook'] = 'field_presave';
- check if the same applies to other field hooks.

User interface changes

none.

API changes

The flag for backward-compatibility must not be written to. As there is no module using this name already no issues are to be expected.

This seems to be the root-cause for problems like this:
#1140188: Fatal errors during or after adding default values for autocomplete widgets

The fact that is not called on default values (field settings form) is reported here:
#1944678: On field settings form, hook_field_load() and hook_field_presave() are not called.
#1899498: Field default values do not get hook_field_presave() run on them

CommentFileSizeAuthor
#5 fields.dot_.png125.4 KBtorotil
field_guarantee_hook_field_presave.patch2.23 KBtorotil

Comments

torotil’s picture

torotil commented
Status: Active » Needs review
David_Rothstein’s picture

David_Rothstein commented

I don't think I understand where there's a bug in the field API here... if a particular entity-providing module doesn't call this function, that seems like a bug in that module.

I guess you're referring to this:

Also field_attach_presave() is not executed at all for default values from the instance settings. This practically makes hook_presave() effectively useless.

But I'm not sure what you mean by that exactly. What is a specific place where "default values from the instance settings" are used and where field_attach_presave() doesn't get called but you think it should?

As for the patch, looks like it would change the order in which several hooks are called, so it doesn't seem backwards-compatible for Drupal 7?

torotil’s picture

torotil commented

The problem here lies with field_attach_instance():

function field_attach_insert($entity_type, $entity) {
  _field_invoke_default('insert', $entity_type, $entity);
  …
  // fields are saved to the database
  …

The first line of field_attach_insert() possibly loads new default values - that were never passed through hook_field_presave() - and in the same function stores them to the field-storage. So even:

field_attach_presave(…);
field_attach_insert(…);

won't execute hook_field_presave() for default values.

torotil’s picture

torotil commented

In general it seems that the field API exposes many opportunities to be broken by modules implementing entities or even just by form-submit-handlers. Moving the call of field_attach_presave() into the field_attach_insert() and field_attach_update() would eliminate one such point. But many other remain.

  • node_preview() calls hook_field_load() for items submitted by the field-widget. See: #1990818: node_preview() shouldn't call hook_field_load().
  • node_preview() applies hook_field_prepare_view() and hook_field_formatter_prepare_view() to items that are later passed to hook_field_widget_form(). - no issue yet opened
  • field_entity fails to call field_attach_presave() - in some cases.

All those errors have something in common: They seem to be bugs in the field implementation. All field types that don't have the same (or a compatible) data structure for items for the widget, storage and formatter, have issues with that … The only reason we don't see this more often is because most of the field_types use the same representation all over the place or have already some workarounds for that lying around.

In my current setups I'm using node, file_entity, user, redhen_contact and message entities. I worked closer with node and file_entity and both have shown field-api breakages - I'm sure I'll find such mistakes in the other entity types as well.

To get back to this bug-reports topic: Field-API provides hook_field_presave() and states that it is called before field items are stored. How can it be not the responsibility of the Field-API to get sure it is indeed called?

torotil’s picture

torotil commented
StatusFileSize
new fields.dot_.png125.4 KB

Here is an attempt to put all the different field-API behaviors into one sketch. It is based on adding a $item['functions'][] = __FUNCTION__; into all hooks mentioned.

johnv’s picture

johnv
Primary language Dutch
commented

IMO these issues are related, if not duplicates:
#1994594: hook_field_presave() not called reliably
#1899498: Field default values do not get hook_field_presave() run on them

benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
commented
Issue tags: +Needs issue summary update

See the instructions for adding an issue summary: http://drupal.org/node/1427826 . There are not a lot of comments here, but some of them look complicated enough that we need a "good parts" version in the summary.

johnv’s picture

johnv
Primary language Dutch
commented
Issue summary: View changes

Updated summary, using template.

johnv’s picture

johnv
Primary language Dutch
commented
Issue summary: View changes

Updated

johnv’s picture

johnv
Primary language Dutch
commented
Version: 7.x-dev » 8.x-dev
Status: Needs review » Needs work

The summary is updated.
#4 is not yet incorporated. @torotil, can you do that?
I marked this as a D8-issue, since this issue contains API-changes, as I described in the summary.

Also, the relation with #1899498 must be cleared.

johnv’s picture

johnv
Primary language Dutch
commented
Issue summary: View changes

Updated

johnv’s picture

johnv
Primary language Dutch
commented

If a field has a hook_field_presave, it is likely to have a hook_field_load().
Added to summary: "The patch must be enhanced with hook_field_load()."

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Version: 8.x-dev » 7.x-dev

8.x works quite different, is called by the entity storage controllers and soon an integral part of those. No idea if this can be done, but I think it can be moved back to 7.x as it is no longer applicable for 8.x.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Issue summary: View changes

The patch must be enhanced with hook_field_load().

torotil’s picture

torotil commented
Issue summary: View changes

Updated issue summary.

torotil’s picture

torotil commented
Issue tags: -Needs issue summary update

I've just updated the summary to reflect the current state.

torotil’s picture

torotil commented
Issue summary: View changes

Fixed a mistake in the issue summary.

torotil’s picture

torotil commented
Issue summary: View changes

hook_field_load is already called automatically in field_attach_load(). Also there is no conflict with #1899498.

Status: Needs work » Closed (outdated)

Automatically closed because Drupal 7 security and bugfix support has ended as of 5 January 2025. If the issue verifiably applies to later versions, please reopen with details and update the version.