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
A recent critical issue (#2766957: Forward revisions + translation UI can result in forked draft revisions) brought up the fact the the 'revision_translation_affected' field is basically required for any translatable and revisionable entity type.
Proposed resolution
Let's make the entity field manager provide this field by default.
Remaining tasks
Review.
User interface changes
Nope.
API changes
Nope.
Data model changes
Nope.
Comment | File | Size | Author |
---|---|---|---|
#45 | interdiff.txt | 1.74 KB | amateescu |
#45 | 2896845-45.patch | 207.63 KB | amateescu |
#42 | interdiff.txt | 8.56 KB | amateescu |
#42 | 2896845-42-for-easy-reviewing.patch | 21.79 KB | amateescu |
#42 | 2896845-42.patch | 208.08 KB | amateescu |
Comments
Comment #2
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedHere's a start. Now that I wrote the patch, I'm wondering whether this should actually be provided by default by the storage, like 'default_langcode'.
Comment #3
plachProbably it should, but we should be able to detect whether a non-core entity type already defined it by itself, otherwise it could be a non-BC change.
Comment #4
plachTests green, but this is obviously not ready :)
I guess we will need also an upgrade path for any non-core translatable and revisionable content type that does not implement the RTA flag. Actually we have
content_moderation_state
in core, but that's internal so not sure how much it matters.Comment #5
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedGoing deeper with this field would mean something like this. No interdiff because this is a new patch :)
Comment #7
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedDamn it, making it an entity key automatically promotes it to 'not null'.
Comment #9
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedI think it makes sense for this to be an entity key. FWIW, you can revert the not null handling like so:
Comment #10
pk188 CreditAttribution: pk188 at OpenSense Labs commentedUpdated according to #9.
Comment #11
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commented#10 doesn't use entity keys which was what #9 was based on.
Comment #12
amateescu CreditAttribution: amateescu for Pfizer, Inc. commented@Sam152, I thought about doing that but I didn't want to increase the number of hacks in that code area :)
One other option would be to make it a revision metadata key but this flag is not really metadata, is it?
Comment #13
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedProbably a better question for @plach, but it seems to fit the definition of "metadata" to me.
Comment #14
timmillwoodIIRC revision metadata can't be translatable.
Comment #15
plachHonestly I don't remember if revision metadata can be translatable, but if it can RTA would qualify in my book :)
Comment #16
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedI just checked and @timmillwood is right, revision metadata field are specifically excluded from being part of any data table, so they can't be translatable :/
Comment #17
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedOk, let's hack the entity keys some more then. It's better than hardcoding the field name everywhere..
Interdiff is to #5.
Comment #19
plachI manually tested #17 and, after running the update, this is what I get:
Comment #20
amateescu CreditAttribution: amateescu for Pfizer, Inc. commented#19 was happening because the update function was using the cached entity type definitions which didn't include the new auto-populated key. Easy to fix though :)
Comment #21
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedDiscussed this issue with @plach on IRC and he suggested the improvements from the interdiff attached.
We also wrote a CR for this issue: https://www.drupal.org/node/2897586
Comment #22
tstoecklerAwesome change, and great work in getting it done!
One comment on the latest interdiff:
I think this deserves a comment. Is the reason for adding this as a sort of "safeguard" that we rather mark all revision translations as affected, rather than not mark some as affected that actually would be affected?
Comment #23
plachYou're right about the comment. It's about being consistent with the previous API return value: if the field was not defined the value returned was always TRUE.
Comment #24
plachMissing trailing dot :)
Setting to NW for #22.
Comment #25
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedI just love copy-pasting @plach's comments :D
Comment #26
plach#21 is green, and #25 contains only comment changes so we should be good here.
CR for this available at https://www.drupal.org/node/2897586
Comment #27
timmillwoodGreat job @amateescu!
+1 to RTBC.
I think we need to remove commit credit for @pk188.
Comment #28
plachWhy? He did work on this issue, even if it was a small reroll.
Comment #29
timmillwoodThe patch wasn't a reroll, it was implementing #9, regarding entity keys, when the patch wasn't using entity keys, as mentioned in #11.
Just trying to crack down in credit for incorrect patches, we had the same issue here too https://www.drupal.org/node/2856363#comment-12140284
Comment #31
catchJust to make sure, since we don't have other revisionable/translatable entity types in core, the update will be a no-op.
But if #2880149: Convert taxonomy terms to be revisionable had landed, then it would run on taxonomy terms.
That makes it less risky to commit during alpha, since it will only affect contrib entity types or altered core ones, but do we need explicit test coverage for the update?
Comment #32
timmillwoodUnless I missed something, this patch will update the Media core entity type, which doesn't have a revision_translation_affected field.
Comment #33
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedWhat happens if this gets in, the update runs, then an issue like #2880149: Convert taxonomy terms to be revisionable lands which changes an entity to be revisionable. Does that leave it with pending entity updates? At that point are they required to install the storage definition themselves, like in system_update_8402?
Comment #34
timmillwood@Sam152 the update to
SqlContentEntityStorageSchemaConverter
in this patch covers that.Comment #35
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedAh, totally missed that. Nice one.
Comment #36
plach@catch
Isn't this enough as explicit test coverage?
Comment #37
timmillwoodAs this is already RTBC it should be fine for 8.4.x
Comment #38
catch@plach it's not confirming that the schema in the db is correct after the update or that the default value is set correctly. Can we load one of the entities and check?
Comment #39
timmillwoodI've been doing some testing this morning and it doesn't look as though system_update_8402 is run for any entity types. node and block_content are the only entity types which make it to the
foreach
, but they already have the RTA field, so don't get it installed.When running \Drupal\system\Tests\Update\EntityUpdateToRevisionableAndPublishableTest none of the entity_test entity types make it into the update.
Comment #40
timmillwoodIt seems as though at the time of running
system_update_8402
entity_test_update entity type doesn't have a revision entity key, so is seen as not revisionable, although in\Drupal\system\Tests\Update\EntityUpdateToRevisionableAndPublishableTest::testConvertToRevisionableAndPublishable
it is seen as revisionable by the end of the updates. So it must not be getting the key added until aftersystem_update_8402
.Think I'm a little out of my depth, and @amateescu is out this week, but I'll check in with @plach once he's online.
Comment #41
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThe analysis in #40 is correct, and the reason we can not properly test this in
EntityUpdateToRevisionableAndPublishableTest
is because we need an entity type that is revisionable and translatable before runningsystem_update_8402()
, but converting an entity type to revisionable uses a post_update hook which cannot run before an update hook.Working on it.
Comment #42
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThere's no other way around it, we need to add a new test db dump which contains entity_test_update entities that are already translatable and revisionable and write a separate test case for it.
Since the full patch contains a test db dump now, I'm also attaching a "for review" patch :)
Comment #43
plachThanks @amateescu, looks good to me! I found only a few minor issues.
Unrelated change?
Missing
Test
suffix in the class name.Can we use the entity type manager here?
Can we use the entity type manager stored in the class property here?
Comment #44
timmillwood@amateescu - Does this mean that if we make an entity type revisionable and publishable (such as comments or taxonomy terms) they will get NULL revision_translation_affected values because they won't have
->setInitialValue(TRUE);
?Comment #45
amateescu CreditAttribution: amateescu for Pfizer, Inc. commented@plach, good catches :) Fixed #43.1, 2, and 4. For 3) we can't use the entity type manager because
\Drupal\system\Tests\Entity\EntityDefinitionTestTrait::updateEntityTypeToRevisionableAndTranslatable
uses the old entity manager :/@timmillwood, nope, we're explicitly setting
revision_translation_affected
to TRUE inSqlContentEntityStorageSchemaConverter::copyData()
and test it inSqlContentEntityStorageSchemaConverterTest
. See the first two parts of the interdiff from #42.Comment #46
timmillwoodAwesome, thanks for the clarification @amateescu!
+1 for RTBC from me then, but I'll let @plach take another look.
Comment #47
plachThanks!
Comment #48
catchThanks for the additional test coverage. Committed/pushed to 8.5.x and cherry-picked to 8.4.x. Thanks!
Also tagged for a release notes mention.
Comment #50
jibranPublished the change notice.
Comment #51
xjmThis also sounds maybe worth mentioning in the release notes?
Edit: Looks like catch also tagged it; not sure how or why the tag didn't remain on the issue.
Comment #52
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedI suspect #49 and #48 were some kind of race condition. The "Fixed" didn't stick around either.