Add migrations for new block editor #7910
Conversation
|
For reference, I've tested all three migrations on a database that was at 8.6.0 (having previously been upgraded from a 7.15.3 database with Stacked Content and color pickers in it), and straight from 7.15.3 to this latest. There were no issues either way, at least with the data I have. |
Shazwazza
added
the
state/sprint-candidate
…anged configuration
|
I'm going to pull the Color Picker migration out of this PR, and move it into a PR for issue #7939, along with migrations for the other items mentioned in that PR. |
|
Stil needs review from @Shazwazza |
umbrabot
removed
the
state/sprint-candidate
There was a problem hiding this comment.
@nielslyngsoe + @benjaminc have left some notes here. I know you have tested and that's good so this is purely just about the code. But there's one question about accidentally breaking peoples sites but maybe you'd know more about that than i do.
| public override void Migrate() | ||
| { | ||
| // Get all document type IDs by alias | ||
| var docTypes = Database.Fetch<ContentTypeDto>(); |
There was a problem hiding this comment.
why not just do Database.Fetch<ContentTypeDto>().ToDictionary(x => x.Alias, x => x.NodeId) ?
There was a problem hiding this comment.
I had written an app a couple years back that did a lot of list to dictionary conversions, and it was performing horribly. After doing using VS's performance analyzer, we found that the dictionary resize operations are extremely expensive, especially as the list grows to 1,000+ elements. If you size the dictionary appropriately at the start and then just insert into it, it was lightning fast. Ever since then, I've always tried to make it a habit to size the dictionary appropriately and then insert into it. Since the ToDictionary extension is on the IEnumerable interface, it can't do that, and so just iterates and adds to the dictionary. If they created a ToDictionary that was an extension on the ICollection interface, that would be ideal, but without that I just create my own, properly sized dictionary and add to it.
There was a problem hiding this comment.
Fair enough! Might be worth making an ext method for this then based on ICollection, not required for this PR but in the future. And good to know! Wonder if there are places in our codebase where we are creating large dictionaries with ToDictionary based on collections, might have a look today and see.
There was a problem hiding this comment.
I decided to create a PR for changing some of our internal usages of ToDictionary and discovered we should be using ToLookup in various places too instead of GroupBy/ToDictionary which is much slower. I've left this open for the community to review but please have a look i fyou have time #7969
|
|
||
| elementTypeIds = elementTypeIds.Union(parentElementTypeIds).ToList(); | ||
|
|
||
| // Convert all those document types to element type |
There was a problem hiding this comment.
Is there any chance this can break people's sites? Like if potentially they were using a doc type as both an element type and a doc type?
There was a problem hiding this comment.
There is, and I thought about that further when I was writing the migrations that I did for #7939 where I decided that if there were any element types already (i.e. this wasn't a v7 to v8 migration), then I wouldn't mess with retyping any document types. I'll port that change back to here, to minimize the effect on anyone upgrading from a previous v8 version. We are dealing just with those upgrading Stacked Content, so likely they are still coming straight from v7 anyway, but it would be a good quick check.
There was a problem hiding this comment.
Cool so since this is already merged i supposed we will just wait until you have a little bit of time to push a new PR, just let us know :) I'll add some notes to the code.
src/Umbraco.Core/Migrations/Upgrade/V_8_7_0/StackedContentToBlockList.cs
Show resolved
Hide resolved
src/Umbraco.Core/Migrations/Upgrade/V_8_7_0/StackedContentToBlockList.cs
Show resolved
Hide resolved
src/Umbraco.Core/Migrations/Upgrade/V_8_7_0/StackedContentToBlockList.cs
Show resolved
Hide resolved
src/Umbraco.Core/Migrations/Upgrade/V_8_7_0/StackedContentToBlockList.cs
Show resolved
Hide resolved
NOTE: This is a review PR to the block-editor-list branch, not a PR to the v8/contrib or other such branch.
This PR adds three database migrations. Two are necessary for converting stacked content data to block list data. The third is one for the color picker, which apparently never got its pre-values converted correctly, and which I found while testing and working with the block list data. I thought about moving that last one into its own issue and PR, but then there would be code merges and DB state merge paths in the UmbracoPlan class, and if no one else is complaining about it yet, I didn't think it worth it.
This item has been added to our backlog AB#6195