Use the actual save function for saving the copy

[patrickdemooij9]

Based on the feedback of @KevinJump in #11867 (comment)

By using the actual Save function of the service, it'll go through all the needed notifications.

[github-actions]

Hi there @patrickdemooij9, thank you for this contribution! 👍

While we wait for one of the Core Collaborators team to have a look at your work, we wanted to let you know about that we have a checklist for some of the things we will consider during review:

• It's clear what problem this is solving, there's a connected issue or a description of what the changes do and how to test them
• The automated tests all pass (see "Checks" tab on this PR)
• The level of security for this contribution is the same or improved
• The level of performance for this contribution is the same or improved
• Avoids creating breaking changes; note that behavioral changes might also be perceived as breaking
• If this is a new feature, Umbraco HQ provided guidance on the implementation beforehand
• 💡 The contribution looks original and the contributor is presumably allowed to share it

Don't worry if you got something wrong. We like to think of a pull request as the start of a conversation, we're happy to provide guidance on improving your contribution.

If you realize that you might want to make some changes then you can do that by adding new commits to the branch you created for this work and pushing new commits. They should then automatically show up as updates to this pull request.

Thanks, from your friendly Umbraco GitHub bot 🤖 🙂

[mikecp]

Hi @patrickdemooij9 ,

Thank for the update, it's indeed best to have the same behavior on all saves 😉

However, while trying to figure out if it was a good idea to have a scope defined in the Copy method, and then another scope created in the Save method that would be called within that "copy scope", my attention was brought to the fact that the scope on the Copy method does not seem to be used anywhere. in the method
So, I was wondering if it would not be actually possible to simply remove the scope in Copy all together...
What do you think? Or am I overseeing something?
Cheers!

[patrickdemooij9]

Hi @mikecp,

The Scope is actually needed here because of the _dataTypeContainerRepository.Get(containerId) call. If I remove the scope, then it doesn't have an ambient scope to use and will throw an exception.
However, I saw that there is an GetContainer function that I can use, so that the scope only gets created if we need it.

I also added the change requested by @ronaldbarendse. I am hoping this isn't considered a breaking change as the interface was changed in 10.3 (as long as we push this one before that). Otherwise I'll have to find a way around it

[patrickdemooij9]

@mikecp
Is this something we might still be able to push with Umbraco 10.3? As I am sure @KevinJump will need this for uSync compatibility

[mikecp]

Hi @patrickdemooij9 ,

Thanks for the update, and sorry for the delay. It's all working fine, so let's get this merged!

I'm putting @nul800sebastiaan in here to get his attention on your 10.3.0 question. I hope we can get it in indeed to avoid the breaking change issue.

Cheers!

[nul800sebastiaan]

So.. I missed this comment, so this didn't make it to 10.3.0. Looks like this is a breaking change now, which we could add for 11.0.0 but unfortunately it was too late for 10.3. In order to not make it breaking we could do it as a default interface implementation, which would be much preferred since we've also released an RC for v11 already, so new additions like this one will be going in 11.1.0.

I'll add a PR to fix it so this is non-breaking.

[nul800sebastiaan]

Unbreaking in #13288

I don't fully understand the scope of the problem, but if we introduced something that breaks uSync, we could call this a regression and add the fix to a 10.3 patch release as well? Does that make sense? // cc @KevinJump

[KevinJump]

Hi,

Yeah, from a uSync (and I guess deploy?) point of view if there are no notifications fired when the copy happens then there is no way to track the changes.

So, if it's gone in to 10.3 without that then - it just means datatypes created by a copy won't be written to disk for syncing (until a save or something else is performed on them by the user).

[nul800sebastiaan]

@KevinJump Makes sense!! Would you mind giving this a test run once the build finishes please? Just as a sanity check for me to know if I didn't do anything silly..? 😅 https://github.com/umbraco/Umbraco-CMS/tree/release/10.3.2

[KevinJump]

Sure @nul800sebastiaan does it publish the results to a nuget feed anywhere?

[nikolajlauridsen]

Not @nul800sebastiaan 😄 but since this is a release branch it'll automatically publish the result to our prerelease feed: https://www.myget.org/F/umbracoprereleases/api/v3/index.json

[KevinJump]

Yeah - seems to work for me from the pre-release feed. 👍

[nul800sebastiaan]

Thanks @KevinJump for the super fast turn-around! 🤩