EntityQuery with condition on the revision field leads to wrong results

I think this is quite a big bug. This also did work before (Drupal 8.0.3).
I attached the patch that I'm currently using to solve this. (I also set ->setRevisionable(TRUE) in baseFieldDefinitions on the revision field on custom entities).

This should apply. Note that the patch is not complete and only works for custom entities, only use it if you need a quick fix.

I know it makes sense for the 'revision' field to be revisionable, but does it *need* to be?
Could we not just add some conditions into the query to make sure it queries the right table?

@hchonov I know. Your patch is more complete. My patch should not be committed to core.
My patch is only for people who need a quick fix for custom entities who don't want to use your patch yet as it might not work correctly as it does some other stuff to existing entities too.

@timmillwood, why shouldn't it be marked "revisionable"? I think that's actually what that property is supposed to be used for.

We should try to follow best practices as much as possible. It looks to me like the property should be marked revisionable. It's probably the more likely solution to get committed to core.

Still needs work as the full patch hasn't passed tests yet.

@hchonov Why does the patch make a change to $fields['langcode'] ? If it isn't needed to solve the problem in the summary it could be moved to a separate issue to simplify this one.

Patch didn't apply. Rerolled after solving conflict on system.install.

I just hit this myself, trying to pull paragraphs data, which heavily rely on older revisions (particularly when they're cloned).

Just a quick request - is there actually an alternate to QueryInterface's condition method that would actually filter on $revision_id? A replacement for:

$query->allRevisions()->condition('type','my_paragraph_type')->condition('id',12345)->condition('revision_id',12346);

That properly works for filtering on non-current revision_ids?

As previously discussed here, this is generating the query:

SELECT base_table.revision_id AS revision_id, base_table.id AS id FROM paragraphs_item_revision base_table INNER JOIN paragraphs_item_field_data paragraphs_item_field_data ON paragraphs_item_field_data.id = base_table.id WHERE (paragraphs_item_field_data.type = 'my_paragraph_type') AND (paragraphs_item_field_data.id = '12345') AND (paragraphs_item_field_data.revision_id = '12346');

Except paragraphs_item_field_data only contains the most recent revision_id. If I could query against the paragraphs_item_revision table instead, the query works, but condition doesn't take table names.

My quick hack is to not filter on revision_id at all, but rather to use $query->allRevisions()->condition('type','my_paragraph_type')->condition('id',12345);, gathering ALL the $revision_ids as [ $revision_id => $id] pairs, then filter on the data manually after the query is complete. Very hacky, and it may scale very poorly but it does work.

I was wondering if there was something else temporary, but more elegant, that didn't involve dumping entityQuery outright.

My org has encountered this same issue. Like TrevorBradley, we've implemented a workaround wherein we have a helper function that does the additional Paragraph revision filtering right after the query runs, but it would be great to have this resolved (or even to have a better workaround, if anyone knows of one). Thanks for the work on this so far everyone!

I was struggling with default_content bugs and revision failures as part of #2698425: Do not re-import existing entities and finally and finally started digging into the entity system and fell into this. Oh man that hurts.

Here's a reroll of #25 working around a bunch of code moving to DefaultTableMapping and moving the update hook.

One other interdiff, I removed the logic exception addition in EntityFieldManager. Even with the update hook, I needed a container rebuild but the rebuild failed building the field definitions. Maybe there's a way to keep that but it seemed like a super painful update. Maybe a deprecation notice would be better? I don't know....

Lets see how tests go. Hopefully just the field changes from the previous patch.

That failed pretty terribly. the revision field is completely removed from entity definitions. I assume I botched rerolling but maybe there's been some fundamental change that breaks this approach. I was dodging quite a few commits trying to resolve where changes where suppose to go and I'm not clear on how the fix works so 50/50 on where the blame lies. If someone who understands the approach wants to give some pointers I can take another stab at fixing this.

Rerolled again as it was not applying.

@gordon. Make sure to move status needs review after applying patch.

Making the revision ID field revisionable seems a bit risky at this point, and, since it's a "special" field (like the ID, UUID and bundle fields), I don't see why we shouldn't hardcode it in entity query itself.

Here we go: #3125912: Consider making the revision ID field revisionable

The code looks good.
The documentation is in order.
The test tests what is should test.
The bug from the IS will be fixed, small little fix.
For me it is RTBC.

Committed/pushed to 9.1.x and backported through to 8.9.x, thanks!

Thanks!