If I use entity fields as output and enable grouping in some occasions I get notices about undefined indexes and broken field output.
Some of these notices can be avoided by adding extra grouping fields in the Aggregation settings of a field.
E.g on a text field you can include "Format" to ensure this field value item is available for later use.
Unfortunately there are some "hidden" field value items e.g. "safe_value" on text fields. Those "hidden" field value items currently aren't addressed by views_handler_field_field.
Those "hidden" field value items are handled by hook_field_load and hook_field_attach_load.
These hooks are fired whenever an entity is loaded.
If aggregation is used views_handler_field_field modifies the field item values, to inject the views specific field output, of the returned entities.
Doing so the field item values are overwritten completely - and thus everything that's not explicitely declared in the Aggregation settings is lost.
The in code documentation says:
// Note: We would copy original values here, but it can cause problems.
// For example, text fields store cached filtered values as
// 'safe_value' which doesn't appear anywhere in the field definition
// so we can't affect it. Other side effects could happen similarly.
As disscues with dereine via IRC we should fire hook_field_load and hook_field_attach_load once the field item values are overwritten, to maintain the "hidden" field item values.
dereine already proposed a solution: http://paste.pocoo.org/show/540775/
The attached patch tries to take care about the // @todo: This hook should actually use the multiple execution.
To do so quite a bit of the post processing in views_handler_field_field was changed, including a function signature.
None
views_handler_field_field::set_items()($values, $row_id) to ($entity_type, $entity, $display).views_handler_field_field.
Comments
Comment #1
das-peter commentedI've just figured out that:
views_handler_field_field::set_items(). A patch to make it working with the new function signature is added.Comment #1.0
das-peter commentedUpdated issue summary.
Comment #2
dawehnerChanging something in this file actually should require some test coverage as this is really a major functionality of views.
Here is an issue with the testing, though you can't say it's started yet.
#722172: Write tests for the field integration
Comment #3
tim.plunkettTriggering the testbot.
Comment #5
yang_yi_cn commentedthe first patch in #1 fixes my problem, which is:
- I have content which has references to Photo (entity)
- Photo entity has an image field
- I ran an aggregation based on location (lat/lng) of the content instead of node id
- the image path is suppose to be pulled using Node > Photo > Image relationship.
without the patch, the image path will only show the image style directory but not the actually file name. With the patch it works.
I do experience a side effect in Link module, though I think it's Link module's problem that didn't expect that _load will be called in other than node_load. I supplied a patch to Link module in http://drupal.org/node/1374928#comment-7080876
Comment #6
Exploratus commentedI have the same problem with images as stated in #5. Will test patch.
Comment #7
Exploratus commented#1 worked for me. as soon as I applied the patch, my images (image cache) started showing up on the aggregated view. The url string was broken before as stated in #5, but now i dodnt get duplicate content AND I can see the images. YAY!
I am not sure this is the correct approach, but at least it works in the meantime, until a better patch is worked through,
Comment #8
das-peter commentedUpdated patch. Here's a nice issue why: #2090885: Views handling of entity references is broken if multiple rows reference the same entity
Comment #8.0
das-peter commentedUpdated issue summary.
Comment #9
valthebaldIt looks like patch doesn't apply anymore (hunk #3 fails)
Comment #10
shashwat purav commented#1 Worked for me. :)
Comment #12
laVera commented1. For users:
The last patch wasn't working for current 7.x-3-dev; Here is one working.
2. For the maintainers:
This is a 3 years old bug, what do you need to commit?
Comment #13
dawehnerYou know, committing is the easy part, but actually thinking about it, is the hard part. What does that mean, if you commit this patch.
The additional hook fired might lead to NETTO more bugs than we fix with this particular commit. Maybe most
hook_field_load()implementations don't think about non actual entities and do something bad with it. Would be nice to have some kind of research or general statement about it.
Comment #14
laVera commentedThanks dawwehner!
I'm not sure what you mean by NETTO but I guess is something like "net total".....
I understand this change may break some other modules, and they will need upgrades. But my point is that this a mayor usage bug:
It break images on products display views, where you do relation ship to ensure product variants are available. Then for less technical site builder, they will go to magento, pimcore or another non Drupal alternative :(
Then, I understand that dev versions are precisely for this kind of think....I don't see a problem with a dev module breaking other modules for fix an own bug, that why is it's dev.
I'm committed to Drupal promotion, but this kind of thing are hinders for new adoption. Please let me know if there is something that I could to help as general user (I have no idea what
hook_field_load()do)Comment #15
colanI'm not convinced we should be making these API changes in D7 just yet. Let's get this figured out in D8 first, and then backport if desired.
Comment #22
panchoComment #28
catchThis is no longer an issue in Drupal 9, there aren't hook_field_load() hooks to trigger separately from field rendering.