EntityFieldHandlerHelper should not use views row_index as entity_id [#1350814]

Problem/Motivation

Currently EntityFieldHandlerHelper::get_value() uses $handler->view->row_index directly as entity id.
I just learned by using the views_content_plugin_style_ctools_context (see line 31-34) that we can't take for sure that the row_index really contains the entity id.
Thus I'd propose to use a construct like following instead:

$entity_id = $handler->view->result[$handler->view->row_index]->entity;

If following changes are made this patch becomes superfluous:
#1368508: Document and enforce nummeric array keys starting at 0 for $views->result
#1368548: Do not index views results by entity id

Proposed resolution

Please see the problem description / patch.

Remaining tasks

Review attached patch.
Check if this approach somehow interferes with #1172970: provide a unified way to retrieve result entities
Check if the other files where the row_index is used need to be modified too:

entity/views/handlers/entity_views_handler_field_field.inc - entity_views_handler_field_field::get_items()
entity/views/plugins/entity_views_plugin_row_entity_view.inc - entity_views_plugin_row_entity_view::get_value()

User interface changes

none.

API changes

none.

Comment	File	Size	Author
#2	entity-EntityFieldHandlerHelper-should-not-rely-on-row_index-as-entity_id-1350814-2.patch	1.89 KB	das-peter
#2
	entity-EntityFieldHandlerHelper-should-not-rely-on-row_index-as-entity_id.patch	1.65 KB	das-peter

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

23 November 2011 at 18:39

Status:

Needs review

» Needs work

The last submitted patch, entity-EntityFieldHandlerHelper-should-not-rely-on-row_index-as-entity_id.patch, failed testing.

Comment #2

das-peter CreditAttribution: das-peter commented 23 November 2011 at 18:43

Status:

Needs work

» Needs review

File	Size
entity-EntityFieldHandlerHelper-should-not-rely-on-row_index-as-entity_id-1350814-2.patch	1.89 KB

grml - seems like today is my sloppy patches day...

Comment #3

dawehner

German

CreditAttribution: dawehner commented 23 November 2011 at 19:25

I'm not sure where $result->entity comes from, but if it's exists it makes sense. $view->row_index does definitive only hold a numeric id for the current row, but not the entity id.

Comment #4

das-peter CreditAttribution: das-peter commented 24 November 2011 at 11:27

Looks like we've to modify search_api as well.

Came across a scenario where the row index is used as entity id. Affected methods:
SearchApiViewsQuery::get_result_entities() and/or SearchApiViewsQuery::get_result_wrappers()

Scenario I'm experiencing:
entity_views_plugin_row_entity_view::pre_render() calls $this->view->query->get_result_entities() which then triggers SearchApiViewsQuery::get_result_entities().

Edit:
Just created related issue in the search_api queue: #1351524: SearchApiViewsQuery::get_result_wrappers(): Do not use views row_index as entity_id.

Comment #5

das-peter CreditAttribution: das-peter commented 26 November 2011 at 10:49

Issue tags:

+dvcs11

Added Views sprint tag.

Comment #5.0

das-peter CreditAttribution: das-peter commented 12 December 2011 at 14:46

Issue summary:

View changes

Updated issue summary.

Comment #6

das-peter CreditAttribution: das-peter commented 13 December 2011 at 07:41

Related to this issue: #1368508: Document and enforce nummeric array keys starting at 0 for $views->result
If this issue is done the requested change here becomes superfluous

Comment #7

das-peter CreditAttribution: das-peter commented 18 December 2011 at 15:07

Priority:

Normal

» Major

Approach was confirmed by merlinofchaos and dereine - better to make the change sooner than later.
Pushing prio.
Let's get #1368548: Do not index views results by entity id done asap.

Comment #8

fago

German

Vienna

CreditAttribution: fago commented 21 December 2011 at 14:44

Assigned:	Unassigned	» drunken monkey
Status:	Needs review	» Closed (works as designed)

I agree that we should not assume that row id == entity id.

However, I was thinking we don't so. Also, when looking at the code we just assume wrappers are indexed the same way as the view row-ids, what should be valid. And we have even documented it that way:

   * @return
   *   A numerically indexed array containing two items: the data type of
   *   the wrappers returned by this method; and the array of retrieved
   *   EntityMetadataWrapper objects, keyed by the same indexes as the results.
   */
  public abstract function get_result_wrappers($results, $relationship = NULL, $field = NULL);

Thus, if that's not the case it's a bug of the implementing module. e.g. search api.