field_views_data() merges in all field data, but not recursively.
Image module's views implementation uses hook_field_views_data_views_data_alter() to add reverse relationships, which is fine because it is operating on a table it doesn't define.

Field Collection's views implementation uses image as a guideline, but the alter is operating on our own table, so this could theoretically be done in the original hook itself, not the alter, which would let other modules more reliably alter it, without worrying about weights.

The current field_collection_field_views_data() adds information about each field to $data, while the alter adds info to $data['field_collection_item']. If the two functions are combined, the value of $data['field_collection_item'] is overwritten by each call to the alter.

Because of the nature of the data being provided, I don't believe the change to array_merge_recursive will cause any problems. Patch forthcoming.

Comments

tim.plunkett’s picture

Status: Active » Needs review
StatusFileSize
new744 bytes

See attached.

fago’s picture

sub

yep, imho views should follow module_invoke_all() here.

merlinofchaos’s picture

array_merge_recursive() can cause arrays to nest too deep if the keys are the same, however, and that can break the data. I don't trust that.

fago’s picture

yep, that's a problem module_invoke_all() faces too, see #791860: array_merge_recursive() is never what we want in Drupal: add a drupal_array_merge_recursive() function instead.
While it's not ideal, I think it makes sense to follow core + it would allow views to avoid implementing its own alter hook.

tim.plunkett’s picture

I didn't realize http://api.drupal.org/api/drupal/includes--bootstrap.inc/function/drupal... actually existed.

It still carries the problem of later hooks overwriting earlier ones, but not in a broken way. Not sure if this is more acceptable.

tim.plunkett’s picture

StatusFileSize
new746 bytes

Reroll with drupal_array_merge_deep().

tim.plunkett’s picture

Should the order of $data and $result be switched? So that $result can only add to $data, not overwrite it?

fago’s picture

I think so. It would be more natural if the first one wins in case of "conflicts".

tim.plunkett’s picture

StatusFileSize
new745 bytes

I thought I'd rerolled this, sorry for the delay.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

From my perspective this looks fine.

dawehner’s picture

Status: Reviewed & tested by the community » Fixed

Thanks commited to 7.x-3.x

Status: Fixed » Closed (fixed)

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