Problem/Motivation

This is a follow up of #1351524: SearchApiViewsQuery::get_result_wrappers(): Do not use views row_index as entity_id. since I've just discussed following with dereine: #1368508: Document and enforce nummeric array keys starting at 0 for $views->result
Reason for this change is that I just came across the fact that views_content_plugin_style_ctools_context (see line 31-34) breaks the "rendered entity" view mode provided by entity_views_plugin_row_entity_view.
Actual scenario:

  • SearchApiViewsQuery::addResults() sets the results keyed by entity-id.
  • entity_views_plugin_row_entity_view::pre_render() stores the entities of the results in an internal array keyed as the views results.
  • views_content_plugin_style_ctools_context::render() resets the array keys of the results.
  • entity_views_plugin_row_entity_view::render() uses the current row index to access the entity in the internal array but because of the reset of the result keys there isn't a match.

Proposed resolution

Use numeric array keys starting at 0 for $views->result in SearchApiViewsQuery::addResults().
The attached patch also contains a chage that could bring a slight performance enhancement by saving a foreach.

Remaining tasks

Review patch.

User interface changes

none.

API changes

none.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

das-peter’s picture

Priority: Normal » Major

Approach was confirmed by merlinofchaos and dereine - better to make the change sooner than later.
Pushing prio.

sanduhrs’s picture

Status: Needs review » Reviewed & tested by the community

Patch looks good and works as advertised.

davidseth’s picture

@das-peter, what are the impacts of this patch? Will any search_api views break? Do I have to re-create anything?

Thanks,

David

das-peter’s picture

@davidseth: Actually this shouldn't have any effect on existing search_api views. The most important thing is that the views and search_api version match - otherwise the view results could disappear.

dawehner’s picture

@drunken monkey/@future-comaintainer
Please leave me a not when you want to commit this patch, so we could sync a release.

drunken monkey’s picture

Sorry for taking so long! As you might know, I was offline due to a hand injury …
The patch looks good, and a duplicate issue (#1623448: SemanticViews row classing breaks under SearchAPI) also confirms this works without problems. I'd be ready to commit this.

But I guess it would be better for me to just commit this, as it works without the Views patch, but not the other way round? When do you plan the next release? I think if I do a new Search API release a bit before (and you only commit your patch right before the release), there shouldn't be too many problems.

dawehner’s picture

Sure no problem!

There is not a real plan but i would like to get a new one out, because it was a lot of time between the last release.
Maybe one could schedule a release for this/next week.

drunken monkey’s picture

Status: Reviewed & tested by the community » Fixed

OK, I now committed this. Thanks, everyone!
I see this module also hasn't had a new release in more than a month, so while normally not necessary we surely could do a new release now. I'd say I'll create one tomorrow, specifically mentioning this issue as being a reason to update soon, and then a few days or a week later you can create a new Views release with that patch.
Seems like a good solution to me.

drunken monkey’s picture

Sorry, I might have forgotten to do this.
I did it now, though, so in a few days it should be at least a bit safer to commit the other patch. Sorry for the delay!

Status: Fixed » Closed (fixed)

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