Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate nested/recursive legacy pre-value property values #71

Merged
merged 4 commits into from
Oct 2, 2024

Conversation

ronaldbarendse
Copy link
Contributor

Although PR #61 added support for migrating pre-value property values, it used artifact migrators inherited from PrevaluePropertyValueArtifactMigratorBase (using Deploys PropertyValueArtifactMigratorBase) that only migrates first level properties on the content artifacts. This was used because property type migrators are only executed when a property editor alias changes, but the following property editor aliases from Umbraco 7 are still used and are therefore unchanged:

  • Umbraco.CheckBoxList
  • Umbraco.DropDown.Flexible
  • Umbraco.RadioButtonList

However, if these property editors are used in nested/recursive property values (like Nested Content), the values weren't migrated from the ;; delimited string to either the single value (for Radio button list) or a JSON array (for Checkbox list and Dropdown list), causing JSON deserialization errors when opening/rendering the content (as reported on umbraco/Umbraco.Deploy.Issues#234 (comment)).

To workaround this, I've added a PrevalueArtifactMigrator that adds a prefix to these property editor aliases on the content artifacts (if imported from Umbraco 7, adjusting the 'original' editor alias of the content when it was exported) and added property type migrators that does the property value migration going from:

  • MigratePrevalue.Umbraco.CheckBoxList to Umbraco.CheckBoxList
  • MigratePrevalue.Umbraco.DropDown.Flexible to Umbraco.DropDown.Flexible
  • MigratePrevalue.Umbraco.RadioButtonList to Umbraco.RadioButtonList

Umbraco 7 also has some other legacy dropdown editors that have changed into Umbraco.DropDown.Flexible, which also requires property type migrators to migrate the property values:

  • Umbraco.DropDownMultiple
  • Umbraco.DropDown
  • Umbraco.DropdownlistMultiplePublishKeys
  • Umbraco.DropdownlistPublishingKeys

And finally, I noticed the DropDownFlexibleDataTypeArtifactMigrator was added after DropDownDataTypeArtifactMigrator, resulting in Umbraco.DropDown data types being correctly migrated into Umbraco.DropDown.Flexible, but then the already migrated data type configuration was migrated again, resulting in an empty configuration.

To easily register all property type migrators, I've added a AddLegacyMigrators() extension method, similar to the artifact migrators. This will need to be included in the documentation, as the property values otherwise won't get correctly migrated (without throwing exceptions during the import).


For testing, I've created a basic Umbraco 7 export that contains a single page with a Nested Content property that has Dropdown lists, a Checkbox list and Radio button list: export-Umbraco7-NestedContent-Prevalues.zip.

This ZIP archive can be imported to a v13 site using the following composer:

using Umbraco.Cms.Core.Composing;
using Umbraco.Deploy.Contrib.Migrators.Legacy;
using Umbraco.Deploy.Infrastructure.Migrators;

internal sealed class LegacyImportComposer : IComposer
{
    public void Compose(IUmbracoBuilder builder)
    {
        builder.DeployArtifactTypeResolvers()
            .AddLegacyTypeResolver();

        builder.DeployArtifactMigrators()
            .AddLegacyMigrators()
            .Append<ElementTypeArtifactMigrator>()
            .Append<ReplaceNestedContentDataTypeArtifactMigrator>();

        builder.DeployPropertyTypeMigrators()
            .AddLegacyMigrators()
            .Append<NestedContentPropertyTypeMigrator>();
    }

    private sealed class ElementTypeArtifactMigrator : ElementTypeArtifactMigratorBase
    {
        public ElementTypeArtifactMigrator()
            : base("component")
        { }
    }
}

Copy link
Contributor

@AndyButland AndyButland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code all looks good, but in running the test steps, although the import looks to complete successfully I see this in the content editor:

image

@ronaldbarendse
Copy link
Contributor Author

Code all looks good, but in running the test steps, although the import looks to complete successfully I see this in the content editor

The values shown are correctly migrated though, so this looks like the content type (with alias component) isn't correctly configured in the Block List data type and/or not marked as element type. Can you verify that and re-import again using the above composer?

@AndyButland
Copy link
Contributor

AndyButland commented Sep 30, 2024

component is an element type but the problem looks to be that it's not defined in the "Available Blocks" section of the "Test - Components - Nested Content" data type. If I add it, then the content looks correct in the backoffice.

@ronaldbarendse
Copy link
Contributor Author

component is an element type but the problem looks to be that it's not defined in the "Available Blocks" section of the "Test - Components - Nested Content" data type. If I add it, then the content looks correct in the backoffice.

I can reproduce this on a clean install and it is caused by the fact Nested Content (in v7) doesn't add dependencies to the used document/element types (with ordering enabled) in the data type artifact... If the data type is migrated before the component element type is deployed, it can't find the element type. This is also the reason why we've added the recommendation to first import only schema, followed by both schema and content in our documentation: if you do this, everything in imported/processed/deployed without issues.

To avoid having to do multiple imports, we could patch Deploy Contrib (v2, as newer Deploy versions do correctly add these dependencies) or manually add an alias to element type key lookup by creating a custom ReplaceNestedContentDataTypeArtifactMigrator and overriding the GetContentTypeKey(...) method...

@AndyButland
Copy link
Contributor

AndyButland commented Oct 2, 2024

To avoid having to do multiple imports, we could patch Deploy Contrib v2

Maybe it's a little odd patching a 5 year old version, but if it's not too difficult to do it might be worth it. As you might well miss the note in the docs. That said, importing from 7 is a one-off task, so if you have to do it in two steps that's not a big problem in my view.

@ronaldbarendse
Copy link
Contributor Author

Maybe it's a little odd patching a 5 year old version, but if it's not too difficult to do it might be worth it. As you might well miss the note in the docs. That said, importing from 7 is a one-off task, so if you have to do it in two steps that's not a big problem in my view.

We only ever released Deploy Contrib 2.0.0 😇 But that would also require users to upgrade their old project, before doing the export... Although they already need to install the Deploy Contrib Export package, so maybe we should include a custom Nested Content value connector and override the default one 🤔 Let's leave this as 'nice to have' for now, because there might be other editors that require a similar fix and doing the import is (mostly) a one-off task as you mentioned...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants