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
I have a non-translatable and non-revisonable entity type that I want to convert into a non-translatable and revisionable entity type. When I use the SqlContentEntityStorageSchemaConverter
to convert my entity type, I get an error, Unknown field revision_translation_affected
.
Proposed resolution
Check if the entity type is translatable before using the revision_translation_affected entity key.
Remaining tasks
Tests.
Comment | File | Size | Author |
---|---|---|---|
#20 | interdiff.txt | 1.49 KB | amateescu |
#20 | 2919157-20.patch | 17.78 KB | amateescu |
#18 | interdiff.txt | 723 bytes | amateescu |
#18 | 2919157-18.patch | 17.85 KB | amateescu |
#15 | interdiff.txt | 27.98 KB | amateescu |
Comments
Comment #2
benjy CreditAttribution: benjy at Unearthed commentedHere's a patch that fixes the error.
One other issue I noticed is that revision_user and revision_created don't get a default value from the original entity?
Comment #3
benjy CreditAttribution: benjy at Unearthed commentedSomething like this fixes that for me.
Comment #4
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedCan we check if the $revision_translation_affected_key field exists instead? Do all translatable entities have this?
I suppose based on #2896845: Provide the 'revision_translation_affected' base field by default for all revisionable and translatable entity types, this isn't a problem for 5.x, but possibly could if this is backported as a bugfix?
Comment #5
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedIt would also be good to run the upgrade path on a non-translatable entity and see what happens.
Comment #6
benjy CreditAttribution: benjy at Unearthed commentedI expected this to pass, it fails but i'm not sure it's for the correct reason. The null field in ContentEntityBase that blows up is
default_langcode
not sure if i'm doing something wrong in the test.Comment #8
amateescu CreditAttribution: amateescu for Pfizer, Inc. commented@benjy, that's probably because
SqlContentEntityStorageSchemaConverterTest
usesdrupal-8.0.0-rc1-filled.standard.entity_test_update_mul.php.gz
as the test database dump, which contains a translatable entity type.A test for this bug would need to use
drupal-8.0.0-rc1-filled.standard.entity_test_update.php.gz
instead :)Also, I agree with #4, let's switch to an
hasKey()
check on the entity type.Comment #9
benjy CreditAttribution: benjy at Unearthed commentedHere's another attempt at a test only patch, it fails for the right reason but you can only see that if you manually tweak the exception thrown in copyData() on line 314 to include the original exception message.
Comment #10
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedGreat, I'm glad that #8 helped you :)
We need to use the temporary entity type here, and the check needs to be on the 'revision_translation_affected' string, not the variable defined outside of the foreach.
We should also check the field values like we do in the other test, which probably means we need an abstract base test class for these two tests in order to share at least that bit of code between them.
Comment #11
benjy CreditAttribution: benjy at Unearthed commentedSorry @amateescu I was meant to say thanks for the help in #8, I got distracted and then came back and just hit post.
The test doesn't pass when using the temporary_entity_type, is that definitely right? I've left as original entity type for now which should pass.
Comment #13
amateescu CreditAttribution: amateescu for Pfizer, Inc. commented@benjy, yes, using the temporary entity type is definitely the right thing to do. However, in this case the 'revision_translation_affected' entity key is always defined, even for non-translatable entity types, so the solution here would be change the
if
check to:I tested locally and the test passes with that change :)
Comment #15
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedImplemented the change proposed in #13 and completed the test changes for #10.2.
Also attaching a test-only patch to show that new test coverage with a base class still catches the bug properly.
Comment #18
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedOops :)
Comment #19
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedOne comment, one style thing and one clarification. If the last point is clarified, this looks great and can be RTBC.
Okay, so the same test with a key difference: different test fixtures. The best docs for the characteristics of each fixture were on the docblock of
_entity_test_update_create_test_entities
. I was somewhat confused why we weren't just using the state based entity overrides, but each fixture does a whole lot more, nice!Could we assert the negative case here too?
$this->assertEquals($translateable, isset(...
We obviously get implicit coverage of this because of the original error from the IS, but it makes the code a bit nicer too I think.
Is there a reason this test isn't applicable to the non-translatable entity type as well? Is that just out of scope here, given the restructured tests provide coverage for the bug in the IS and don't touch the error handling?
Comment #20
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThanks for the review!
Re #19:
1. Yep, those fixtures were quite hard to create, and I had to document the process otherwise no one else would be able to touch them in the future :)
2. That's very clever, I hope I understood your point correctly in this interdiff.
3. The problem with that test method is that it asserts entity UUIDs, and those are different in the two fixtures. So I thought that having explicit if/else blocks is not really worth it :)
Comment #21
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedNice, this looks great to me.
Comment #24
catch