Summary

Canvas is phasing out page regions and introducing page variants. Canvas Translate hardcodes support for regions but not for variants.

Steps to reproduce

Proposed resolution

Variants should be supported.

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

gábor hojtsy created an issue. See original summary.

gábor hojtsy’s picture

Status: Active » Needs review

Now has a full upgrade path too :) Should have been needs review all along :)

lauriii made their first commit to this issue’s fork.

lauriii’s picture

Title: Page variants are not supported » Add support for Page variants
Category: Bug report » Feature request
Priority: Normal » Major
f0ns’s picture

I 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.

f0ns’s picture

I'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.

f0ns’s picture

Heads-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.

f0ns’s picture

Status: Needs review » Needs work

Found some things that I want to look into and resolve.

f0ns’s picture

Status: Needs work » Needs review

Fixed and all green, more information on the MR.

f0ns’s picture

Nice 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.

f0ns’s picture

Status: Needs review » Needs work
f0ns’s picture

Correction 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.

f0ns’s picture

Status: Needs work » Needs review
f0ns’s picture

Pushed 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.

f0ns’s picture

I 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 cst shows 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::IMPORT subscriber 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

lauriii’s picture

Thanks for the review! I think those would be reasonable changes to make here. Happy to review those!

f0ns’s picture

Pushed:
- 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.

f0ns’s picture

Pipeline 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.

lauriii’s picture

Status: Needs review » Fixed

Opened a change record for this to explain the upgrade path. Thank you @f0ns and @Gabor Hojtsy for the help!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

f0ns’s picture

Nice! I'm so happy I can stop thinking about this one. 🥹

Thanks all!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.