Cloning feature with fallback for 1.10.x

[MatteoPiovanelli-Laser]

Since we discussed cloning in the context of localization in #7352, I checked what the cloning feature does in dev.
The goal here is to make the cloning feature by @TFleury (see 87a3948 and #7400) retrocompatible to what is in 1.10.x.

In order to do that, I pulled those commits and reverted everything that was not directly related to Cloning (hence the number of commits in this branch's history).

Additionally, I introduced a fallback logic in the driver coordinators for cloning (see both ContentFieldDriverCoordinator and ContentPartDriverCoordinator):
The idea is that is a driver does not explicitly implement Cloning, the coordinator will clone the part by using the "old" logic of export/import. As a consequence, if we want a part (or field) to not be cloned, in at least one of its drivers we should declare a Cloning override that does nothing (e.g. see AutoroutePartDriver here).

I think with this fallback the cloning feature introduces no breaking changes to 1.10.x, but I'd like to have other people's opinions, because I might have missed a lot of things.

note:
I reverted most changes in the Orchard.Localization module, even though I liked them better than what's in it right now, to avoid mixing too many concerns in a single pull request. (Also, I'd rather have a Translating feature, that falls back to cloning as a default, than a Cloning feature that may optionally also do translation, but this is not the place to go on about this)

[sebastienros]

You should squash all the changes since master in order to remove all these useless commits and comments.
If you don't know how to do it just let me know.

[MatteoPiovanelli-Laser]

@sebastienros please, help me squash that mess I pushed

[sebastienros]

Checkout your branch.

git reset --mixed 1.10.x

This will move the HEAD of your branch to 1.10.x, without changing any file. It means that now git sees all the changes you made ready to commit as one set of changes. Verify all the changes are as expected or fix it.
Commit your changes, you'll get a new commit with everything on your working branch.
Force push to your repository and the PR will be updated automatically with it.

[MatteoPiovanelli-Laser]

I think I did it!
Thanks @sebastienros

[sebastienros]

And now that it's clean, we can see the breaking changes ...

IContentFieldDriver and IContentPartDriver new methods.

[HermesSbicego-Laser]

I see...
My thoughts:
we have various unresolved problems around translations/localizations in 1.10.x.

1. First of all, we should try to have a working branch for 1.10.2 with basic translations working. I mean working at least as in 1.8; Cloning or not, we should choose a way, then my team can work on it.
2. That done, I think my team will be able to push PR in order to manage proper localization for cpf, taxonomies, terms, blogs, blogposts, culture neutral fields for 1.10.2; otherwise, it will be difficult.

@sebastienros, @MatteoPiovanelli-Laser can we talk about a strategy tomorrow during or before triage?

[sebastienros]

I will join 15 minutes the meeting if you want. Ping me if I forget.

[HermesSbicego-Laser]

@MatteoPiovanelli-Laser creating a derived interface like

public interface IContentFieldDriver2 : IContentFieldDriver {
void Cloning(CloneContentContext context);
void Cloned(CloneContentContext context);
}

And then to implement it where necessary, can solve or
make matters worse?

[sebastienros]

I'd say IContentFieldCloningDriver ,same for parts, inheriting from IContentFieldDriver.
And in the dev branch we remove it.

[MatteoPiovanelli-Laser]

@sebastienros @HermesSbicego-Laser
Let me see if I understand what you are saying (I'll talk about parts, but the same stands for fields):
Having the new Cloning and Cloned methods in the IContentPartDriver is breaking, because of course it is, and I missed it.
So, what you are proposing is to create the new IContentPartCloningDriver to define those methods, and implement that in ContentPartDriver.

The same does not hold, however, for IContentHandler, because that has Cloning and Cloned already, and that means that I probably should not, right now, be changing the ContentPartDriverCoordinator further.

Does that sound right?

[MatteoPiovanelli-Laser]

I pushed the change with those two new interfaces.

I haven't yet checked that all the modules in the repo implement the new methods as needed. The ones in this PR are those that had them implemented in dev, but we should do a check on all modules and features just in case, especially in light of the fact that, because of the fallback, if we need a specific part/field to never be cloned, we should implement an empty Cloning method there.

@sebastienros chat with you today 1145am PST?

[MatteoPiovanelli-Laser]

@sebastienros I made the change we discussed in the driver coordinators. It still works on my machine.

[MatteoPiovanelli-Laser]

@sebastienros
I will try to explain the reasoning behind those two lines where I used reflection in the driver Coordinators. (I will talk about fields, but exactly the same goes for parts). This way, you can probably be quicker in finding where the issue lies that had me use reflection.

ContentFieldDriver implements the IContentFieldCloningDriver interface. It also has the virtual methods Cloning and Cloned, whose implementation here is empty (no code, so clone nothing). Any driver implementing the abstract class ContentFieldDriver will have concrete implementations of those virtual methods: these may either be declared in the concrete driver, of be the same as the methods from the abstract class.
The reasoning behind the fallback logic is:
If any of the concrete drivers for a specific field have actually implemented the Cloning method, we will execute that. Otherwise, if none of the concrete drivers have it implemented, we will run the fallback mechanism, doing Export/Import. The way I check whether the drivers have implemented the method is using reflection (because nothing came to mind that would not break the interfaces): I check whether the concrete driver declares its own override of the abstract Cloning method.

Why did I implement this in the coordinator like that?
Initially, I had thought of implementing a fallback by coding the fallback directly in the virtual Cloning method from the abstract class ContentFieldDriver. However, I decided against it because that would give me issues when processing a field that had multiple drivers, where not all of them have a concrete Cloning override: if at least one driver had no override for Cloning the fallback would be run at least once, possibly causing issues. That would be especially an issue in light of retrocompatibility, because no driver that is out there based on 1.10.x right now implements its own Cloning methods.

Here's something I realized after implementing this:
This mechanism of course becomes troublesome when we want to use the Cloning for translations. Suppose we implement a Cloning method in our driver, that only does something when there is the parameter telling me that we are translating. Then, when we try to simply clone the field (actually, clone an item that has it) in the coordinator we will find it has its own cloning method declared, and thus we will not run the fallback: if we were expecting the export/import to happen, now it won't.
Example:
Suppose for localization we use Cloning with the additional property. For ContentPickerField, I will go and implement that in the driver from feature we added recently. The original driver will remain untouched, with no override of Cloning. When we are doing a new translation, the items selected will be (for example) replaced by their correct translations. When we want to clone, however, we would like the Export/Import to happen, because that has always worked well enough for us. Now, since we implemented the override in the second driver, the fallback will not happen, and since we do not pass the property for translation, that override will do nothig. The result will be an empty ContentPickerField in the clone.
On top of that, I still think it is more elegant to have Translating/Translated methods (defaulting to Cloning/Cloned), but this is totally a matter of preference, I think.

I will be back in a few hours to read any comment, and I will try to implement any alternative you propose before you log back in on Wednesday morning (your morning, my evening).

Cheers,
Matteo

[MatteoPiovanelli-Laser]

@sebastienros I may have found a way to remove the reflection. I will push it on a separate branch later today. Please, let me know if you have a few minutes to discuss it when you log in.

EDIT:
I pushed on https://github.com/LaserSrl/Orchard/tree/CloningFeatureWithFallbackNoReflection
See in particular these files:

• IContentFieldCloningDriver.cs and IContentPartCloningDriver.cs: Added the IsDefaultCloningImplementation readonly property to both interfaces.
• ContentFieldDriver.cs and ContentPartDriver.cs: implemented the new property.
• ContentFieldDriverCoordinator.cs and ContentPartDriverCoordinator.cs: used the new property instead of reflection.

I will explain the reasoning for parts, but it's the same for fields:
The new property is implemented in the abstract class ContentPartDriver in such a way that it is only set to true when the virtual Cloning method runs. Classes inheriting from ContentPartDriver do not have access to the private bool field for which the new interface member is only a getter.
In the coordinator, I first run all the Cloning methods (for a given part). If the combination of all the IsCloningDefaultImplementation for the drivers for that part is true it means none of the drivers implemented its own Cloning method (note that those drivers cannot write on the new property): in that case, I run the fallback using Export/Import: since we've only run the virtual Cloning from the base abstract driver, nothing was cloned, so running the fallback doesn't risk overwriting anything.

I tried this a bit, and it seems to give the same results I got with the version of the coordinators that used reflection.

Issues may arise if peple start doing their own IContentPartCloningDriver implementations and don't respect the same convention on IsDefaultCloningImplementation. If in an implementation of that interface someone does something in the Cloning, and still sets things so that IsDefaultCloningImplementation reads true, the fallback will run and possibly overwrite things.

[sebastienros]

The simplest way to remove reflection is just to check if the instance of the driver IsAssignableFrom an IContentPartCloningDriver , and if so cast it to it. Don't need to check for methods.

[MatteoPiovanelli-Laser]

But doing that check and casting does not take care of a situation where the driver is an implementation of IContentPartCloningDriver, but does not implement a custom Cloning method, which is the most common case to handle for retrocompatibility.

[MatteoPiovanelli-Laser]

Unless we say that abstract ContentPartDriver does not implement IContentPartCloningDriver, and add something like:

public abstract class ContentPartCloningDriver : ContentPartDriver, IContentPartCloningDriver {
    //implementation of cloning
}

And then every where we want a driver to have a "real" cloning behaviour, separate from export/import, we implement ContentPartCloningDriver, and then if we do not override Cloning it just means we want nothing to happen.

However, that looks kind of kludgy to me.

[sebastienros]

What about this:

If someone wants to implement custom cloning, they implement IContentPartCloningDriver also.

From the coordinator (the service that calls the drivers) in the Clone methods, cast each driver to IContentPartCloningDriver with the as keyword, and call its Clone methods if the result is not null, or the fallback behavior instead.

[MatteoPiovanelli-Laser]

Also, if we did as per my last comment, we would run into trouble when we implement cloning+translations.

Say we want the export/import to run when cloning, but we want to do something special when translating, as could easily be the case for e.g. ContentPickerField. If we did things like I described above, we would have to make sure and implement something giving the same end result as export/import in the cloning:

public class MyDriver : ContentFieldCloningDriver<MyField> {
//...

    public override Cloning (Correct parameters) {
        if (translating) {
            //do special things
        }
        else {
            //replicate what doing export/import does, because if you don't
            //no cloning will happen, since the fallback won't run anyway.
        }
    }
}

[sebastienros]

In your example you could have Cloned/Cloning in the base class, and you can just call base.Cloned in the else statement.

[sebastienros]

Try to have the default behavior in the base classes (field and driver). If someone implemented IContentPartDriver directly (without the abstract class) then you can detect it from the coordinator and also call the fallback behavior. So the fallback code is in both the abstract drivers and the coordinator.

[MatteoPiovanelli-Laser]

What about this:

If someone wants to implement custom cloning, they implement IContentPartCloningDriver also.

From the coordinator (the service that calls the drivers) in the Clone methods, cast each driver to IContentPartCloningDriver with the as keyword, and call its Clone methods if the result is not null, or the fallback behavior instead.

Does that mean the abstract class should not be implementing IContentPartCloningDriver?

In your example you could have Cloned/Cloning in the base class, and you can just call base.Cloned in the else statement.

Try to have the default behavior in the base classes (field and driver). If someone implemented IContentPartDriver directly (without the abstract class) then you can detect it from the coordinator and also call the fallback behavior. So the fallback code is in both the abstract drivers and the coordinator.

But the problem with implementing the export/import fallback down in the Cloning of the base abstract class is that then, when we have multiple drivers for a part or field, the fallback may overwrite whatever we did with the Cloning from other drivers.

[MatteoPiovanelli-Laser]

@sebastienros
Re-reading you comments, I think I should stop working in the evening, because yesterday I failed to understand what you had in mind.
Here's what I understand now:

Current state of 1.10.x:
IContentPartDriver -> abstract ContentPartDriver : IContentPartDriver
I would have to add:
IContentCloningPartDriver : IContentPartDriver -> abstract ContentCloningPartDriver : ContentPartDriver, IContentCloningPartDriver

ContentCloningPartDriver has the default implementations of the Cloning and Cloned methods, that is empty: if a part's driver implements ContentCloningPartDriver but does not override the methods, they do nothing.

In the coordinator, when the Cloning is called, I check all the drivers for a part: if none implements IContentCloningPartDriver I run the fallback; else, if even one driver implements the interface, I run their cloning method. If whoever wrote that part's IContentCloningPartDriver implementation didn't override the Cloning method, I assume they understood that as a consequence nothing would happen while Cloning, just like if they didn't Implement Exporting nothing would be exported.
That way, if down the line we have a special member of CloneContentContext to mark translations, I don't have to worry about the Cloning method from the concrete driver doing nothing for "simple" cloning.

Sorry if I misunderstood yesterday. I will code this now and get it pushed on a branch in our repo, so that you can give it a look.

Matteo

[MatteoPiovanelli-Laser]

in the same branch I linked yesterady (https://github.com/LaserSrl/Orchard/tree/CloningFeatureWithFallbackNoReflection) I implemented what I explained in my previous comment. If I try to start a PR it says it can be merged into 1.10.x, but I didnt make a PR yet: if you think that approach is fine, I might overwrite it on the branch from this PR here.

It works on my machine.

Wouldn't this approach for the fallback be incompatible with what is happening in dev?

cc @HermesSbicego-Laser because I'm on vacation starting tomorrow and the whole next week.

[HermesSbicego-Laser]

@sebastienros any update on this?

[MatteoPiovanelli-Laser]

@sebastienros @HermesSbicego-Laser I am back from vacation and ready to go ahead with the discussion on this

[BenedekFarkas]

@MatteoPiovanelli-Laser it's a bit unfortunate, but due to the amount of time passed, do you think this should still be taken care of for 1.10.4? Localization changed a lot on 1.10.x/dev (mostly thanks to you guys) and 1.11 will soon™ be released.

[MatteoPiovanelli-Laser]

We should check whether dev has the fallback. Without it, it would be too much of a breaking change, I think.
Ideally, in my opinion, 1.11 would have the fallback, so that anyone who upgrades would still have a working clone/translation, without the need to implement new specific handlers for it.

[BenedekFarkas]

You mean an automated copying of infoset properties in a similar fashion as ExportInfoset and ImportInfoset? Btw we're currently reviewing/testing your other PR on cloning (#7592).

[MatteoPiovanelli-Laser]

the fallback I have does Export/Import using the existing functionality if no Cloning implementation is found.
The code we have in our live environments looks for Drivers implementing IContentPartCloningDriver. If none are found it uses an Export/Import. Otherwise it uses the Cloning implementations. Same concept for fields. It is slightly different than what is in this PR, because the plan here was to not have a different interface for drivers: here we used reflection to see whether any driver implemented Cloning Explicitly.

[BenedekFarkas]

@MatteoPiovanelli-Laser is this PR still interesting for you? If so, we can discuss details and I can review, but please merge from 1.10.x and/or allow edits by maintainers if it's OK for you that I'd push changes.

[BenedekFarkas]

@MatteoPiovanelli-Laser is this PR still interesting for you? If so, we can discuss details and I can review, but please merge from 1.10.x and/or allow edits by maintainers if it's OK for you that I'd push changes.

@MatteoPiovanelli-Laser ? Should we work on this PR or just #7592?