Closed (fixed)
Project:
Canvas Translate Extension
Version:
1.0.x-dev
Component:
Code
Priority:
Major
Category:
Feature request
Assigned:
Unassigned
Reporter:
Created:
2 Sep 2026 at 16:46 UTC
Updated:
20 Oct 2026 at 19:35 UTC
Jump to comment: Most recent
Comments
Comment #3
gábor hojtsyNow has a full upgrade path too :) Should have been needs review all along :)
Comment #5
lauriiiComment #6
f0ns commentedI tested MR !9 (552e4ab) against Canvas 1.x HEAD and 1.12.0.
I upgraded a real site from Canvas 1.10.1, with Dutch translation state seeded on the Olivero regions, in both orders: both modules updated ( Canvas first followed by Canvas Translate).
What works
Drafts from several regions merge into theme_olivero, a disabled region keeps its draft.
Published translations carry over, including one published after the Canvas update.
Approval is dropped when one moved draft wasn't approved. The dashboard hides regions once default_page_variant is set.
Kernel tests pass.
A bug I found
The moved variant draft only contains entries from regions that had a draft.
Because ConfigTranslatableGlue::publish() replaces the whole component_tree override with the draft, publishing the migrated draft deletes published translations of regions that had no draft.
As a result in my test the published "Powered by Drupal" translation was gone from the variant after publish.
Comment #7
f0ns commentedI've added a fix to the MR.
mergeOverrides() now runs before moveDrafts().
When the move creates the variant draft (the variant had none), also fill in the published text of components whose region had no draft.
Components whose region had a draft are left alone, so deliberately cleared text stays cleared.
Includes the kernel test testMovedDraftKeepsOtherRegionsPublishedText (fails without the fix) and a spec scenario.
All 44 kernel tests pass, phpcs is clean and openspec validate --specs passes.
Comment #8
f0ns commentedHeads-up: #3627678: Fix the phpcs, phpstan and cspell failures in CI makes the phpcs, phpstan and cspell jobs blocking.
Pushed a small fix so this MR stays green once that lands.
Comment #9
f0ns commentedFound some things that I want to look into and resolve.
Comment #10
f0ns commentedFixed and all green, more information on the MR.
Comment #11
f0ns commentedNice fix with the UUID alignment, much cleaner than the stash.
Two things still need to be addressed imo:
1. onConfigImport() runs upgradeAll() after every import, and region overrides are never removed after merging.
If an editor clears all strings of a component on the variant, the next cim restores the old region text, and active config drifts from the export. Should merged region overrides be removed, or should the merge only run when the import created the variant?
2. The skip during sync in DraftCleanupHooks was there for the recreate case, which the UUID alignment now prevents.
I don't think it's needed any longer, and it leaves orphaned drafts when an import really deletes a template or region.
Comment #12
f0ns commentedComment #13
f0ns commentedCorrection on point 2: the skip also protects drafts when an outdated export deletes the variant, so it's better to keep it.
Orphaned drafts there are harmless.
Comment #14
f0ns commentedComment #15
f0ns commentedPushed a fix for point 1.
Tested manually on a site with Dutch translations:
- drush deploy with a different variant UUID in the export: UUID aligned, nothing imported, translations kept, config clean.
- Cleared a variant translation, then an unrelated cim: stays cleared, config clean (before the fix it came back).
- Export with a changed region translation: reaches the variant, via drush cim and via the UI batch import, also when the batch spans several requests.
Comment #16
f0ns commentedI took another look at MR !9 with a fresh head.
Tests and CI are green and the UUID alignment works well now.
I only see one issue and it's the same pattern as yesterday's revert:
The import-time merge in
PageRegionTranslationUpgrade::onConfigImport()doesn't survive a second import.It writes variant translations into active config that aren't in the export. After the first import,
drush cstshows the variant's language override as a pending delete, and the next import removes it.Confirmed with a kernel test that imports twice (my own commit 0931156 doesn't fix this either).
Proposal: drop the
ConfigEvents::IMPORTsubscriber and keep the insert hook and post_update 0002.The subscriber handles two cases, and neither needs it. An import only creates the variant if you run cim before updb, which doesn't work with Canvas 1.11 anyway. An export taken before the update should just be exported again after updb.
Two small things while we're in there:
moveDrafts()keeps an approval when the draft it approves changes: when a variant's own draft gets region entries added, and when a region draft is only partly moved.The MR description needs an update: no more string types, and Canvas >=1.11 is now required.
Happy to push the fix if that sounds right @lauriii
Comment #17
lauriiiThanks for the review! I think those would be reasonable changes to make here. Happy to review those!
Comment #18
f0ns commentedPushed:
- Dropped the config import subscriber; imported variants keep the translations from the export (test imports twice, config stays clean).
- Approvals are cleared when moving drafts changes them.
- Updated the openspec spec to match.
Ready for review.
Comment #19
f0ns commentedPipeline is blocked by an unrelated CI failure (composer artifact too large, 413). Fix in #3628831: Composer CI job fails with 413: artifact too large; will rebase once that's in.
Comment #21
lauriiiOpened a change record for this to explain the upgrade path. Thank you @f0ns and @Gabor Hojtsy for the help!
Comment #23
f0ns commentedNice! I'm so happy I can stop thinking about this one. 🥹
Thanks all!