We always take the entity id:
list($entity_id) = entity_extract_ids($entity_type, $entity);

If the table is a revision table, we should take the $revision_id instead, because the rows are keyed by that.

Same thing needs to happen in views_megarow_preprocess_views_view_table().

The best way would be to implement a isRevision (or similarly named) method on the field handler, and use that to determine which id to take in both places.

Of course, page callbacks that need to load the entity will need the "is_revision" flag passed, along with the entity type.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

bojanz’s picture

Title: The links field handler doesn't support revisions » Doesn't support revisions

Changing title, since the problem exists in the module as well.

peter-boeren’s picture

Hey Bojanz,

I just ran in to this issue. Has there been any progress on this? Because for a project we would like to integrate Megarows with workbench. A few of the views Workbench supplies are based on revisions. If no work has been done, could you provide a few handles so we can implement the changes ourselves?

vasike’s picture

Title: Doesn't support revisions » get_result_entities() Doesn't support revisions
Project: Views Megarow » Views (for Drupal 7)
Version: 7.x-1.x-dev » 7.x-3.x-dev
Component: Code » Views Data
Category: bug » task
Status: Active » Needs review
FileSize
7.02 KB

it seems like Views issue.
bojanz pointed to an 8.x-3.x-dev Views issue (#1758634-15: The query plugin should load entities after the query has been executed) and its solution http://drupalcode.org/project/views.git/commit/6661020ca5ba7d5c41d2ec74b...

here is a patch that aims to adapt bojanz solution for 7.x-3.x-dev.

Status: Needs review » Needs work

The last submitted patch, result_entities_for_revision-1802438-3.patch, failed testing.

vasike’s picture

Status: Needs work » Needs review
FileSize
6.94 KB

patch corrected

Status: Needs review » Needs work

The last submitted patch, result_entities_for_revision-1802438-5.patch, failed testing.

peter-boeren’s picture

Hi Vasike, I will test your patch soon. Thanks in advance for the effort.

vasike’s picture

Status: Needs work » Needs review
FileSize
7.1 KB

here is a new patch. move query groupby, after getting base field for entity table.
i hope it will pass the tests.

Status: Needs review » Needs work

The last submitted patch, result_entities_for_revision-1802438-8.patch, failed testing.

vasike’s picture

Status: Needs work » Needs review
FileSize
7.51 KB

new patch - get_result_entities() in execute()

Status: Needs review » Needs work

The last submitted patch, result_entities_for_revision-1802438-10.patch, failed testing.

andrewbelcher’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
2.7 KB

Hopefully I'm not missing anything as my patch is significantly smaller and simpler than some of the others (it's not a direct port of the D8 stuff, more just trying to make D7 work with revisions)... But here is a simple patch.

- We add the $data[TABLE]['table']['revision'] key (node is the only core entity that supports revisions right?)
- If the table we're working on is the revision table, we load via node_revision_load() if the entity type is a node, otherwise we look for entity so we can use entity_revision_load(). If we can't get either, we trigger a WATCHDOG_NOTICE explaining the problem.

This seems a simple enhancement that isn't going to break anything core, but enables other modules to do a lot more (for example VBO can make itself work with Search API - #1334374: Re-use generic entity views table).

andrewbelcher’s picture

D'oh, node_revision_load() is D8 only, have updated the code for the somewhat hacky D7 equivalent...

Status: Needs review » Needs work

The last submitted patch, 13: 1334374-25-generic_entity_tables_revisions.patch, failed testing.

andrewbelcher’s picture

Sorry, not having a good end of the day, uploaded the wrong files... Here we go...

The last submitted patch, 13: 1334374-25-generic_entity_tables_revisions.patch, failed testing.

stefanotabarelli’s picture

Assigned: Unassigned » stefanotabarelli
Status: Needs review » Reviewed & tested by the community

We have been using this patch for more than 10 months on our platform and it seems to work really well so I wanted to give credit to the author and update the status of this patch to RTBC.

DamienMcKenna’s picture

Assigned: stefanotabarelli » Unassigned

dawehner’s picture

Status: Reviewed & tested by the community » Fixed

Its good to know that we have someone actually manually using that patch. Its a bit sad what is needed to support revisions, but this is life.

Committed and pushed to 7.x-3.x

Status: Fixed » Closed (fixed)

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