Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
An entity query with a condition on a revision metadata key / field leads to e.g.
Drupal\Core\Entity\Query\QueryException: 'revision_created' not found
The problem relies in \Drupal\Core\Entity\Query\Sql\Tables::addField()
where when calling $table = $this->ensureEntityTable($index_prefix, $sql_column, $type, $langcode, $base_table, $entity_id_field, $entity_tables);
we are not providing the entity revision table when not querying "all revisions".
Proposed resolution
Remaining tasks
User interface changes
API changes
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#20 | 2986887-20.patch | 3.64 KB | tstoeckler |
#20 | 2986887-20--tests-only.patch | 2 KB | tstoeckler |
Comments
Comment #2
hchonovComment #3
hchonovComment #5
hchonovComment #6
tstoecklerGreat find, also nice test! Looks pretty great to me, just one question:
Should we not check that the field storage is revisionable and maybe also that the field name is a revision metadata key?
Comment #7
hchonovI don't think that this is necessary, but if we want to be explicit, then it works for me :).
Comment #8
tstoecklerThanks, yeah that looks good to me. Will give one of the other entity maintainers the chance to take a look at this as well, but for me this is good to go.
Comment #9
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedLooks good to me as well. This patch shows how badly
\Drupal\Core\Entity\Query\Sql\Tables
needs to be rewritten after we finish all the issues from #2960147: Finalize the entity storage :/Comment #10
hchonovI've just realized that the
$field
parameter of\Drupal\Core\Entity\Query\Sql\Tables::addField
is not always simply the name of the field, but it might be[field name].[property name]
, so if we have a field with multiple properties as revision metadata key e.g. entity_reference_revisions, then the current code won't work.Therefore we should retrieve the field name from the field storage definition Instead of using the
$field
parameter.Comment #11
amateescu CreditAttribution: amateescu for Pfizer, Inc. commented#10 makes perfect sense.
Comment #12
alexpottJust thinking but might this be better to change:
to
That way we'll get one less join. OTOH this will lead to always using the revision table even when not needed so that's probably wrong. How about:
Change this to assign $data_table instead. The revision table contains all the data from the non revision data table right? And then we won't have to join across the three tables and have the later if.
Comment #13
hchonov@alexpott, if I understand you correctly then you are asking if the tables are identical and that the revision table only has one more column for the revision ID? If this is the question, then the answer is no.
We might have revision table and revision data table, which are different. If the entity type is both translatable and revisionable then there will be a revision and revision data tables, in which case the revision table will contain all the revision related information only - e.g. revision metadata keys, ID, revision ID and langcode, the revision data table will contain only the revisionable fields without the revision metadata fields. Additionally there will be a data table, which will contain the revisionable fields of the current default entity revision together with the non-revisionable fields. Note that non-revisionable fields aren't placed in any revision table, but only in the base or data table.
See
\Drupal\Core\Entity\Sql\SqlContentEntityStorage::getTableMapping()
for more information.Comment #14
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedI would really prefer not to try and optimize
\Drupal\Core\Entity\Query\Sql\Tables::addField()
at this point, it needs to be completely rewritten after #2960147: Finalize the entity storage.Comment #16
tstoecklerRandom failure.
Comment #17
alexpottThis patch causes
Drupal\KernelTests\Core\Entity\EntityQueryTest
to fail.Comment #18
tstoecklerThe new test method needs to be marked as
@legacy
now due to #2936511: ContentEntityType::getRevisionMetadataKeys() never considers the BC logic because of the new "revision_default" key.Re-RTBCing.
Comment #19
alexpott@tstoeckler it's not a legacy test though. Legacy tests will be removed in D9 and this shouldn't be. We need a non legacy test.
Comment #20
tstoecklerRe #19: See the comment in
EntityTestMulWithRevisionLog
for the reasoning:In other words, all tests that use that entity type are "inherently" legacy, because they implicitly test that BC layer. That is very unfortunate, I absolutely agree. But it seems that was a conscious decision in that issue so it seems kind of out of scope of this issue to revert or change that decision.
Luckily, though, the test can actually be written to use
entity_test_revlog
(i.e. the non-translatable "sibling") to avoid this problem entirely. So here's an adapted patch including a test-only one to prove that it correctly fails like this, as well.Comment #22
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedVery nice! #19 has been addressed wonderfully, so back to RTBC :)
Comment #23
alexpottCommitted and pushed 78da9a1dca to 8.7.x and fcac0e25f8 to 8.6.x. Thanks!
Comment #27
BerdirYay, one bug down.
Looks like querying by revision id field is still broken though: #2766135: EntityQuery with condition on the revision field leads to wrong results