OrchardCMS / Orchard

Orchard is a free, open source, community-focused Content Management System built on the ASP.NET MVC platform.
https://orchardproject.net
BSD 3-Clause "New" or "Revised" License
2.37k stars 1.12k forks source link

Enhancements/Questions on Localization #7352

Open HermesSbicego-Laser opened 7 years ago

HermesSbicego-Laser commented 7 years ago

Hi guys, I have a suggestion/question for you, related to how localization in Orchard works: in particular I would like to know which plans does the community have in order to improve it... some examples, to be more specific:

  1. Container/Containable items have no automations in order to keep container and containable items synchronized based on their culture
  2. Same for Taxonomies and terms, Taxonomy picker field, Content Picker Fileds and so on

It would be nice to have a site settings (or any more granular setting) that tells to our site/part/field what to do if it finds translated contents

We did something for taxonomies/terms/taxonomy field and ContentPickerField for 1.8.2; our approach unfortunately has some negative effects because we had to suppress some default behaviour of Orchard (drivers and handlers); for example a behaviour we had to override was in taxonomy: when a translation for a taxonomy is created the modules creates a new

taxnomy.Name + " Term"

content Type, so we had a proliferation of unnecessary ContentTypes "ProductTypeTerm", "TipoProdottiTerm", "TypesDeProduitsTerm", and we could not accept this effect (collateral effect due to how taxonomies was designed)...

However, being localization a crucial feature of CMS, we would like to know your opinion on that in order to plan our next steps

Another philosphical question: should be the Orchard.Localization Module manage how other modules should react with localizable related contents? Or should every module be designed to consistently manage localization? Personally I prefer the second approach, maybe with some point of extension. Not sure how: IEventHandler called by LocalizationController? adding ContentPartDriver method? What do you think?

Thank you for sharing your opinion with me.

sebastienros commented 7 years ago

I will setup a design meeting to talk about localization.

HermesSbicego-Laser commented 7 years ago

@sebastienros , nice. We will partecipate, probably @MatteoPiovanelli-Laser and me. If it's possible we will prefer a date after November 4... tnks

MatteoPiovanelli-Laser commented 7 years ago

Hello. @sebastienros, is there any news on the discussion about localization?

sebastienros commented 7 years ago

Is November 4th ok for everyone, anytime between 10am PST to 1pm PST (18 to 21 in Central Europe right now)

sebastienros commented 7 years ago

/cc @jersiovic @urbanit

urbanit commented 7 years ago

It works for me.

urbanit commented 7 years ago

I mean 10am pst works for me...

jersiovic commented 7 years ago

Ok 10am pst also works for me

jersiovic commented 7 years ago

To be sure we don't forget points that IMO are interesting here it is a list of needs that IMO is desirable localization solves:

LOCALIZATION NEEDS

  1. To localize data for some fields of a content item.
  2. To share unlocalized data between all cultures of a content item.
  3. To have permissions per culture (or unlocalized) of a content item.
  4. To be able of publishing and set author data only of one culture (or unlocalized)
  5. When localizing taxonomies that has exactly same terms for each culture but with translated captions:
    • a. To don't need one taxonomy type per culture
    • b. To be able of change the hierarchy of terms easily for all the cultures at the same time.
    • c. To be able of removing a term and its localized versions easily.
  6. To be able of query localized content items by culture through Projections and through Content Manager
  7. To be able of query content items filtering by its pointed localized content through Projections and through Content Manager (We need all localized contents of a pointed content is considered the same content).

    Example: You have a non-localizable BoatOffer type with:

    • A Content Picker field pointing to a localizable content items of Boat type.
    • A Taxonomy field pointing to a localizable term of a taxonomy. It is desirable Orchard out of the box allows us to filter boatoffers by a term of a localized taxonomy through Projections and through ContentManager without adding extra complexity.
  8. To be able of grouping content items by its pointed localized content.

    Example: For the previous example we need an efficient solution which provides BoatOffers grouped by pointed taxonomy term considering same term all those localized terms of the same term.

  9. Help to maintain things simple in scenarios where you reference localized content items.

Example: You have class OrderType containing OrderLines with content items ids pointing to content items of type Product. It is desirable code used to deal with Products (filter, group, access data) would be the same if Products is non localizable initially and in the future is changed to localizable. With current solution problem after you make Product localizable with another culture different than the one used by default is you need all products duplicated and need to deal with this duplication everywhere.

  1. Allow to work with taxonomies with different hierarchies per culture.
  2. Allow to work easily with content items that are not localized in all cultures.

LOCALIZATION SCENARIOS

  1. Contents that is expected they will have all its localized fields fulfilled for all the cultures of the site.
  2. Contents that are only available for certain cultures.
jersiovic commented 7 years ago

This is my current view of the two solutions discussed by the moment in relation to the NEEDS defined previously. Maybe it is interesting for the Friday discussion.

LOCALIZATION SOLUTIONS

CulturePart pointing to related localized Content Items

Pros:

To-Do:

Cons:

A content item contains all its localized data

Pros:

To-Do:

Cons:

When we edit a content item with localizable data through Admin UI we will see non localized data and localized data can be seen/edited using a combo to choose the culture in which we want to see those fields.

When editing a content item, those fields we don't have permissions to modify will appear in readonly mode.

-- EDITED --

Save and Publish buttons will store or Publish content item of only data in the culture for which the user has permissions. Only data of cultures with any non-blank data fulfilled will be marked as published. That means when you don't fulfill data for one culture that Content item won't be considered as published for that cultured. Alternatively we can offer a setting at conten type level that mark its Content items always available for all cultures no matter what we fulfill when we store it.

jersiovic commented 7 years ago

Please take a look to this interesting video from the Drupal community related with this topic: Vimeo presents: Drupal 7/8 Content Translation models http://vimeo.com/27068202

HermesSbicego-Laser commented 7 years ago

Works also for me 10am pst.

sebastienros commented 7 years ago

Meeting notes

default: en-US

Page content type With no fields (body, title, url) Home page / -> en-US (id = 10) -> (Id=10 , TSId = 10) Pagina de inicio /pagina-de-inicio -> es-ES (micId: 10) Id =11, TSId = 10 Pagina di inicia /pagina-di-inicia -> it-IT (micId: 10) Id =12, TSId = 10

Product, Title, Weight field, not localizable, Draftable

Motorbike -> Weight: 50kg -> Helmet Motocicleta (es-ES) -> Weight: 50kg -> Casco Motocicleta (it-IT) -> Weight: 50kg -> Casci

O1

Master content item id === Translation sets ids

-- Different menu structures

"Menu English" mi1, mi2

"Menu Italian" mi1, mi2

(Localization Part to Menu Widget)

New Menu Widget

"English menu everywhere" Menu -> Menu English Localization part Culture -> English

New translation "English menu everywhere" Menu -> Menu Italian Localization part Culture -> English

-- Same menu structure (TODO) Translate menu items Localization part to "Menu Items"

Taxonomies: Same as Menu items?


Issue, we only have a MSCId if the localization part is defined

Cid Mcid 1 1 2 2 3 2 4 NULL 5 NULL

MatteoPiovanelli-Laser commented 7 years ago

Hi,

@HermesSbicego-Laser and I debriefed offline a bit. We have some comments on what we discussed regarding the ContentPicker. Specifically, the following points from @sebastienros' notes:

How to select a content neutral content item (from any culture, not culture specific, i.e. the TS id)

  • We need to select the MCI id

The Content Item Picker field shows a drop down with all languages (can/should we store the MCId also?)

  • Option (Settings) to ensure the selected items have the same culture as the current culture
    • Sub-option to translate the selection automatically when a localized version is created

Adding the dropdown with the language selection in the "list" window that appears when clicking "Add" for a ContentPickerField is pretty straightforward, and does not actually require any dependency with the Orchard.Localization module (like it's done in the contents list view).

The Option and Sub-option I think will require a dependency from Orchard.Localization. To avoid introducing that dependency to the Orchard.ContentPicker feature (thus avoid back-compatibility issues) we would go and create another feature in the Orchard.ContentPicker module. Something along the lines of Orchard.ContentPickerLocalizationExtensions. In that feature, we would add the settings we discussed, and the corresponding functionalities. A lot of those we actually have already as a custom extension we had built in O1.8, so it should not take too long to get a PR out for that. Some questions remain for the sub-option:

That feature, as described, should pose no compatibility issue. I think it may not even have to change any existing code-files in the Orchard.ContentPicker module.

Tomorrow I will start working on this, because I have previous work we did to start from and adding the rest that we discussed should not be too much work. Any input and comments are appreciated.

Best, Matteo

MatteoPiovanelli-Laser commented 7 years ago

Hi,

I forgot to discuss a point in my previous comment.

How to select a content neutral content item (from any culture, not culture specific, i.e. the TS id)

  • We need to select the MCI id

I would like a bit more input on it, but this is what I have in my head: This, I think, should again be enabled/controlled by a setting. I think it would override the "list" view, in the fact that it would remove the dropdown for language selection. The list would then only show a single item for each translation set. Q: How should I handle contents with no localization part? Should I even show them at all? I would have a further setting here for this. With this setting enabled, the choice of localization for the selected items is actually handled when displaying. The way I would go about this is I would process things in an alternate view (but that sounds inefficient) or more likely by overriding the OnGetDisplayShape handler. Q: How should I handle the case where the desired localization is not available? I would do as for the sub-option (see my previous comment) and have another setting to allow the user to choose.

I will work on this functionality right after the things I discussed in my previous comment. Here as well, any input and comments are appreciated.

Best, Matteo

HermesSbicego-Laser commented 7 years ago

/cc @sebastienros @urbanit @jersiovic @MatteoPiovanelli-Laser Hi all, can we schedule next meeting for discussing open points on how localization should work in taxonomies/blogs/tags/etc? What about on Thursday 10am pst (18pm Central Europe) before triage meeting?

In the meanwhile I have a question for @sebastienros : We talked about "Culture Neutral Fields" but we didn't threat "Culture Neutral Parts": for parts things are more complex, I think it should be nice to have this feature too, maybe using or extending the cloning logic?

MatteoPiovanelli-Laser commented 7 years ago

I opened #7386 because things in the localization part work different than what I expected/remembered

MatteoPiovanelli-Laser commented 7 years ago

7388 adds the dropdown to select/filter by language in the view opened by the content picker field

MatteoPiovanelli-Laser commented 7 years ago

In light of #7386, @HermesSbicego-Laser and I thougth that it's probably better to add a property TranslationSetId to LocalizationPart, as was initially proposed at the meeting (i do not remember who proposed it). That would have the value of the Id of the first ContentItem in a translation set. All new logic would be built around that, without touching the existing ones that revolve around the MasterContentItemId, to avoid problems with existings modules that might be using that. My idea is to never again use MasterContentItemId after that, but to leave it in for compatibility.

HermesSbicego-Laser commented 7 years ago

Hi all, our team de-de-briefed around previous meeting. we share our notes and considerations on ContentPickeField (we are working on it) and we tried to imagine a similar approach with taxonomies considering that we should not introduce breaking changes

De-briefing notes Content Picker Field (CPF)

Options as discussed during online meeting Otp1: Check culture integrity Opt2 > Sub Opt1: Try to translate automatically

Scenario: Opt1: true Opt2: false

Problem: What to do with Untranslatable Contents (without LocalizationPart)? possibility:

  1. Keep them untouched, they live in CPF
  2. Add Opt1.1 telling if this is an accepted behavior or not (not=Error on publish)

Scenario: Otp1: true Opt2: true

Problem: What happens if a translation is missing? Possibility:

  1. Keep them, save the content and notify it BUT this is nonsense for Opt1 A possible way is to threat Opt2 like an option, I mean not dependent from Opt1 SO Options change Opt1: ... Opt2: ...
  2. Remove them But things change without user consent
  3. Block the save/publish This respect Opt1
  4. Add an Opt3 telling what to do SO Options change Opt1: ... Opt2: ... Opt3 > Sub Opt2 : Keep|Remove

We think we should take this approach, 'cause it supports all scenarios we talked till now So, next in CPF Orchard Module, respecting but also extending the design: Opt1: Try Translate (default true) Opt1.1: Unselect item if translation is missing (default false) Opt1.2: Unselect item if it's neutral (default false) Opt2: Ensure language are the same (default false) Opt2.1: Ensure selection of a culture neutral item is invalid (default false), i.e. you need to select a localizable content item

Update Optionally: Display a drop down in front of the selector to translate the current selection in any other language.

Example make a translation to IT(Itinerary IT) Related POIS (CPF)

Poi1 IT Poi2 en > want keep Video (culture neutral) > want remove Poi3 IT

Opt1: true (want to translate) Opt1.1: false (want to keep untranslated) Opt1.2: true (want to remove culture neutral) Opt2: false (want to keep untranslated) Opt2.1: false (as parent)


Taxonomy How Craft CMS works Taxonomies == Categories

Categories are:

  1. Hierarchical
  2. Are Culture Neutral
  3. Terms are Localizable 3a. When first term is created all localized version are created 3b. When adding a language, a job adds all localized versions in each term 3c. Hierarchy is automatically syncronized too
  4. Taxonomy Field has setting to choose if: 4a. Keep in sync with container 4b. Force a locale

Problems with this approach Can’t manage different hierarchy for different locales Example: In Europe e can sell french cheese In USA we can’t sell french cheese

Q: Do we really want to force this behaviour in Orchard? A: Our position is NO, cause it's wiil limit something that now is flexible

Creating automatically Localized version is not orchard default for the other contents

Problems in current Orchard behaviours When a translation for a taxonomy is created the module creates a new taxnomy.Name + " Term" content Type, so we had a proliferation of unnecessary ContentTypes "ProductTypeTerm", "TipoProdottiTerm", "TypesDeProduitsTerm", and we could not accept this effect (collateral effect due to how taxonomies was designed)... If you attach LocalizationPart to the 'xyz Terms Type' things do not work bacause when you make a translation

Proposed approach LocalizationPart attached to taxonomy (Is optional) LocalizationPartAttached automatically to “all xyz Term” Types, defined into a Site Setting

Taxonomy translation (obviously only if I have the LocalizationPart) Example Tax IT > Create “Tax IT Term” Type (if removes is soft deleted so we can always discover type)

I do Translate Tax EN > it should not create “Tax EN Term” type but it should use “Tax IT Term” as Type

Corner case:

Q: What happens if I attach LocalizationPart on Taxonomy, later? A: When I save a Taxonomy, It should move (and only move) only Terms of same locale to keep Locale in sync

Term translation (obviously only if I have the LocalizationPart) Example Tax IT > create Term: If Term lang != Tax lang If exists parent in same language > it should Change parent to localized parent If does not exist parent in same language > Dont save OR Save as root (You can always move it to its parent later) Should move term in the taxonomywith the same lang If found > it does it If not found translated taxonomy> throw an exception (AddModelError) If Taxonomyis Culture neutral (LocalizationPart is missing) > Attach to neutral Taxonomy TODO next: What happens when I move a Term? Should I move also their translation version? Keep the choice to the user? With a checkbox Try to keep hierarchy in sync

TaxonomyField translation TaxonomyField has a TaxonomySource defined in the TaxonomyFieldSetting

Settings

Opt1: Try Translate (default true)

On creating TaxonomyField

I don’t know the lang so it exposes Taxonomy terms defined in field settings (as is) On editing of a Content TaxonomyField

If Opt1 == true > Should expose Taxonomy terms of same lang of content

If Taxonomyis translatable >

    if Has Translation, Change Taxonomy
    if Has no Translation, it sets taxonomy source to null, Warning on UI as is

If Taxonomyis not translatable > Keep Taxonomy

If terms are translatable >filter Taxonomy Terms on language else >as is

If Opt1 == false > it exposes Taxonomy terms defined in field settings (as is)

On Saving Content Taxonomy Field

If Opt1 == true && (Terms are Translatable || Taxonomy is Translatable) && ContentItem Is Translatable ==> Try to translate terms and removes them if missing

else ==> saves terms (as is)

Problems with current UI When Autocomplete && User can create new terms Our idea is: User Can create new terms only if “xyz Term” is Culture neutral


Blog/BlogPosts Using same approach of Taxonomies/Terms.

If locale of Blog and BlogPost are not the same, tries to move BlogPost under a the right Blog translation but if it can't leave things untouched.


Tags TBD


Container/Containable TBD

sebastienros commented 7 years ago

@jersiovic @urbanit @Xceno

We intend to do the Taxonomy design meeting tomorrow at 9am PT, i.e. 6pm CET

urbanit commented 7 years ago

Unfortunately, it will be impossible for me to attend, at least the 1st hour. I will try my best to join later...

sebastienros commented 7 years ago

Update on Taxonomies

TERM CONTENT TYPE Instead of creating a content type for the terms dynamically, we could first create the type, and select it when creating a new Taxonomy.

Q: Can we change the Term Content Type of a taxonomy localization? No, because when translation an item we don't know yet it's language, hence the term type to create. We need to only be able to create in the same content type as the source taxonomy.

Localization strategies

FIGURE 1

Taxonomy named LETTERS, not localizable Terms localizable

I can't create B_IT as there is not A_IT When displaying the taxonomy for FR I can see all terms, in IT I can only see C_IT

Taxonomy Field

jersiovic commented 7 years ago

I'm just reading now @HermesSbicego-Laser notes and date for the second meeting. I will think about your notes this weekend. Great effort guys!

jersiovic commented 7 years ago

@sebastienros For taxonomy field I would add some customization through field settings:

Setting ii) helps us to simplify queries over those not localizable content items when we want to filter by term and we don't want to do extra work for filtering by all the possible translations of a term.

HermesSbicego-Laser commented 7 years ago

@jersiovic, If I understand well you would add two settings in order to manage the situation in which you have a Culture neutral Content Type and a taxonomy field whose source is translatable:

  1. Add a Field setting in order to show the taxonomy in "current culture" as source of a taxonomy field in the Admin. Q1: When you say "current Culture" you mean the culture selected within the Admin culture picker or the "default culture" of site (defined in "site settings")? Q2: When this setting is true the user cannot choose a different culture?
  2. Add a Field setting in order define the culture of the "Terms Contents" that will be stored in the Taxonomy field: Q1: So when the content is saved, it stores the term ids corresponding to the langauge defined in the setting even if another language is choosen? In that case we force to store term ids in a particular language.

Am I right?

jersiovic commented 7 years ago

Yes you are right.

Answer to 1.Q1: Yes when the user is in dashboard the current culture is the one selected though admin culture picker. Answer to 1.Q2: Yes that is the idea. Answer to 2.Q1: Yes this is it.

HermesSbicego-Laser commented 7 years ago

I don't think we have never used a similar approach but it makes sense for me to have this particular behaviour in order to manage all scenarios, what do you think @sebastienros, @urbanit, @MatteoPiovanelli-Laser? By the way, we plan to begin working on taxonomies from tomorrow and we will push a PR as soon as possible.

MatteoPiovanelli-Laser commented 7 years ago

@HermesSbicego-Laser @jersiovic I have concerns about the scenario where the current culture gets changed between edits of a ContentItem that has a TaxonomyField set as per setting 1.

MatteoPiovanelli-Laser commented 7 years ago

@sebastienros @jersiovic @urbanit @Xceno @HermesSbicego-Laser I am starting on the TranslationSetId (because of #7348, #7386 and #7394). I think I will then work on the synchronization for Neutral ContentFields and ContentParts, that would definitely need those issues to be solved for consistency.

For synchronization, we also discussed cloning, so I opened #7409

HermesSbicego-Laser commented 7 years ago

@sebastienros, we started working on Taxonomies Localization (and on Blog/BlogPost also in order to discover problems there) and we are facing so many problems; we branched it from 1.10.x and the missing of the cloning logic create a lot of problems because translated contents are created "anemic" (they lose all context informations); this affects at least all ContentItems which need to be related to a Container and are not Creatable (Terms, BlogPosts, and so on)... So what I will do is to reintegrate in 1.10.x the work done by @MatteoPiovanelli-Laser on backward compatibility of the cloning feature (#7409) in order to start from a less buggy branch.

IMHO we discovered a really serious bug because from 1.9 what worked into 1.8 stopped working.

sebastienros commented 7 years ago

If the regression you are talking about is the fact that translating an item loses all its data, it's a known issue. Yes, feel free to continue working on backporting the cloning features, I already commented on Matteo's PR.

HermesSbicego-Laser commented 7 years ago

yes, I see. That's fine, I will Update you during tomorrow meeting.

HermesSbicego-Laser commented 7 years ago

In order to manage Blog/BlogPost I have a PR ready based on "#7409".

  1. I created a new feature into Orchard.Blogs named Orchard.Blogs.LocalizationExtensions
  2. I used same approach discussed for the relationship between Taxonomies and Terms. So, on saving the BlogPost if the locale of Blogand BlogPostare not the same, it tries to move BlogPostunder the right Blogtranslation but if it can't leave things untouched.
  3. I created also a migrationwhich attach LocalizationPartto both Blogand BlogPost.

When #7409 will be ready and tested I will merge and push the PR.

jersiovic commented 7 years ago

@HermesSbicego-Laser @jersiovic I have concerns about the scenario where the current culture gets changed between edits of a ContentItem that has a TaxonomyField set as per setting 1.

What kind of concerns? You are worried cause not saved changes will be lost? If it is the problem I think it is not a big problem. Someone editing a non localizable content item usually doesn't need to change the culture cause there is not other translations of the same content item, he only wants to see all the UI and contents in the language he understand.

MatteoPiovanelli-Laser commented 7 years ago

@jersiovic maybe I misunderstood what you proposed. I will try to explain the way I understood things, so you can see what I may have gotten wrong. Please, correct me.

Setting 1 I mentioned was for the case where the ContentType that has the TaxonomyField is not localizable (does not have a LocalizationPart): by enabling setting 1 we are causing the TaxonomyField, when we are editing a ContentItem, to only show, as available for selection, Terms whose culture is the same as the Current Culture. Say for example I have ITEM-A that is of one such type, my Current Culture is it-IT, and in my "Sports" TaxonomyField I select the Term "Calcio". All is fine. Now I change my Current Culture to en-US. I open ITEM-A in Edit again. My TaxonmyField "Sports" is still set to "Calcio", that is my Italian term. However, I now cannot select Italian Terms, so if I make any change, I cannot select "Calcio" again, and I am stuck with a different list of terms.

jersiovic commented 7 years ago

@MatteoPiovanelli-Laser Yes you are right.
An alternative could be to show the master term when the translation in current culture is not available. But IMO this extra work is not worth because is confusing for user and also for the admin of the site to understand what is happening. The point is the use of Setting 1 has only sense for taxonomies where you plan to have all the terms of your taxonomy translated to all available cultures. If not this settings is not useful and is weird someone is willing to use it. Maybe a tooltip in the setting saying: "Use only with taxonomies with terms translated to all available cultures" will help to avoid misunderstandings. I think it is better to left it as you describe it: the user can't select the term because it is not on current culture. In that way the user understands that sth goes wrong with translation of that term and admin of the site the same.

MatteoPiovanelli-Laser commented 7 years ago

In #7416 I implemented the LocalizationSetIds we discussed early on.

I am now using that and #7409 to build the synchronization for CultureNeutral Fields and Parts.

MatteoPiovanelli-Laser commented 7 years ago

I actually have a question, because I forget what we meant by this:

  • How to select a content neutral content item (from any culture, not culture specific, i.e. the TS id)

    • We need to select the MCI id

@sebastienros @jersiovic @urbanit @HermesSbicego-Laser I don't remember what we meant by "culture neutral content item". Isn't that just a ContentItem that does not have a LocalizationPart? Or are we saying that an item that has a LocalizationPart can be marked as CultureNeutral? Does that even make sense? If that is the case, what happens to any localization it may have, or that we may create for it?

urbanit commented 7 years ago

Hi @MatteoPiovanelli-Laser , I think that we mean that we may need to select a specific content item (without take into account if it has localisation info attached or not, and maybe from diffrent culture...) We described the need to choose a specific content item...

MatteoPiovanelli-Laser commented 7 years ago

@urbanit That may have jogged my memory, and I think you are right. So that is not something related to synchronization of parts and fields.

urbanit commented 7 years ago

I think it is more of translating/cloning issue than of synchronization.

jersiovic commented 7 years ago

@MatteoPiovanelli-Laser @urbanit It was related with what I wanted in an scenario where we have a non localizable content item with a content picker field pointing to localizable contents. I wanted to store in the field picker the Translation Set Id but not any concrete content item id of the translations available. The purpose was that when the pantent content item is rendered or edited the content item picker field shows the child content item with the stored Translation Set Id and same culture you are using to render the parent content item.

But if you implement Setting 1 and Setting 2 we talked previously in this thread then I don't need this.

MatteoPiovanelli-Laser commented 7 years ago

@urbanit @jersiovic Ok, I understand now. Thanks.

MatteoPiovanelli-Laser commented 7 years ago

Hello,

I have the synchronization working (on my machine) on a branch based on the PRs I did for LocalizationSets and Cloning. (branch: https://github.com/LaserSrl/Orchard/tree/CultureNeutralPartsAndFields)

I had to make a few changes that should not break anything. I had to add string properties to CloneContentContext, ExportContentContext and ImportContentContext to avoid synchronizing fields that should not be synchronized. For the ExportContentContextand ImportContentContextclasses, I put that property in a base class they now (in my branch) inherit from, because of the way the ContentFieldDriver works (see the Process methods).

In light what I did for the cloning fallback, I don't feel that adding a parameter to CloneContentContext to mark the fact that we are using it for a translation is enough. I would directly add Translating/Translated methods, falling back on Cloning/Cloned if not implemented, with their own TranslateContentContext. I see how that would be a mess, at least for retrocompatibility, in terms of ContentHandlers (I think I would have to do an interface like ITranslatingContentHandler, inheriting from IContentHandler, and then implement from there similarly to what is in the cloning pr for IContentCloningPartDriver) and drivers (IContentTranslatingPartDriver and so on...), but it would also allow really neat stuff. TL;DR: I would have a Translating driver method, defaulting to Cloning, that in turn defaults to Export/Import.

@sebastienros Do you think we'll be able, at today's meeting, to give a quick look at LocalizationSetIds and Cloning?

sebastienros commented 7 years ago

Yes, please demo it.

On Nov 22, 2016 7:19 AM, "Matteo Piovanelli" notifications@github.com wrote:

Hello,

I have the synchronization working (on my machine) on a branch based on the PRs I did for LocalizationSets and Cloning. (branch: https://github.com/LaserSrl/Orchard/tree/CultureNeutralPartsAndFields)

I had to make a few changes that should not break anything. I had to add string properties to CloneContentContext, ExportContentContext and ImportContentContext to avoid synchronizing fields that should not be synchronized. For the ExportContentContextand ImportContentContextclasses, I put that property in a base class they now (in my branch) inherit from, because of the way the ContentFieldDriver works (see the Process methods).

In light what I did for the cloning fallback, I don't feel that adding a parameter to CloneContentContext to mark the fact that we are using it for a translation is enough. I would directly add Translating/Translated methods, falling back on Cloning/Cloned if not implemented, with their own TranslateContentContext. I see how that would be a mess, at least for retrocompatibility, in terms of ContentHandlers (I think I would have to do an interface like ITranslatingContentHandler, inheriting from IContentHandler, and then implement from there similarly to what is in the cloning pr for IContentCloningPartDriver) and drivers ( IContentTranslatingPartDriver and so on...), but it would also allow really neat stuff. TL;DR: I would have a Translating driver method, defaulting to Cloning, that in turn defaults to Export/Import.

@sebastienros https://github.com/sebastienros Do you think we'll be able, at today's meeting, to give a quick look at LocalizationSetIds and Cloning?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/OrchardCMS/Orchard/issues/7352#issuecomment-262268569, or mute the thread https://github.com/notifications/unsubscribe-auth/ABHJ7RFhsW2z-arkgsKf56we4_ZoaD6Aks5rAwf0gaJpZM4KgBIS .

MatteoPiovanelli-Laser commented 7 years ago

@sebastienros Would it be ok if I showed the branch where I have the synchronization? If I have to demo LocalizationSetIds and Cloning separately, it would take a while on my machine to rebuild the different branches and set things up again. Or would you like me to demo the synchronization as well?

sebastienros commented 7 years ago

I don't understand what branches you are talking about, I'll just trust your judgement.

MatteoPiovanelli-Laser commented 7 years ago

Hello.

After a busy stretch (and some vacation) we are back to work on localizations.

I wil try to recap where we stand and our next steps. Please, pitch in with comments.

The things that were left hanging from December, I think, are:

Localization Sets See #7386, #7394, #7348, #7416 There are issues in the way the MasterContentItem is handled in the Localization module (in 1.10.x). In the discussions related to localization, it was proposed to have an Id describing what we called a Localization Set. This is implemented in #7416, by adding a LocalizationSetId property to the LocalizationPartRecord, and handling the logic on that. However, in that commit we introduced an additional service (ILocalizationSetService) rather than correct the behaviour of the existing ILocalizationService. This was done for retrocompatibility: the LocalizationService has been in its (possibly bugged, see #7394) state for a long time, and I would expect that people using it have their own workarounds in place, so "simply" fixing that may break logic depending on the service. @HermesSbicego-Laser discussed this offline with @sebastienros, and it is my understanding that this solution is troublesome in that it introduces a duplication of some functionality between the ILocalizationService (using MasterContentItemId) and the ILocalizationSetService (using LocalizationSetId). A proposed solution to this is that we can fix the behaviours revolving around the MasterContentItemId in the LocalizationService: since this would potentially be a breaking change, we would push this to the dev branch, to be reintegrated later. How does this sound? Should we make these changes to the LocalizationService and push a PR targeted to the dev branch (and maybe to 1.11)?

Cloning See #7409 The thread for that Pull Request has a long discussion on this. Briefly, I took the cloning feature that @TFleury had implemented from the dev branch, and worked on making it retrocompatible with 1.10.x so that it could be used in localizations. The result of that is the introduction of new interfaces for drivers, in order to avoid modifying the pre-existing ones, as well as new abstract driver implementations. Using those, I introduced a fallback behaviour in the driver coordinators, so that if a part's (or field's) drivers do not implement the cloning interface, the cloning is still performed using Export/Import, as used to be the norm. The implementation was initially done using reflection, but there is a version that does not use that (see https://github.com/LaserSrl/Orchard/tree/CloningFeatureWithFallbackNoReflection) that I can push to the PR. I can see some issues in bringing the pr to dev, because it may break the cloning that was done there in the PR by @TFleury (even though I don't think it is actually merged in dev right now). The thing is, if the cloning/cloned methods are in the IContentPartDriver, rather than being declared in a separate interface, I am not sure how to do the fallback without using reflection. Assuming for a moment we removed all the reflection, would this be ok to be merged in 1.10.x? Personally, I like the approach of having additional interfaces and abstract classes to add to the drivers, because it looks easy to expand upon down the line without breaking anything.

Culture Neutral Synchronization As was discussed in an earlier post, I have an implementation for this behaviour based off Cloning and Localization Sets. Once we finalize a decision on those, I can quickly put together a PR, and then we can discuss this in more detail.

Translating The reason we discussed and worked on the cloning was because we plan to use it to prepopulate some parts/fields when doing translations, and also to synchronize parts/fields across a Localization Set. The idea is to modify the CloneContentContext by adding properties to it to mark a Cloning "event" as a translation and behave accordingly. This is doable. However, in light of what I did for the synchronization of CultureNeutral Parts and Fields and to remove reflection from the cloning fallbacks, I do not think this is enough. Basically, my idea is that Translating should default to Cloning, that in turns falls back on Export/Import. If we use a condition in Cloning to do Translating, than we may have a case where:

My idea then is then to create an IContentTranslatingPartDriver , abstract ContentTranslatingPartDriver , and probably a TranslatingPartDriverCoordinator (which would require, I think, to have an IContentTranslatingHandler and a TranslatingHandlerBase) (similarly for fields), that allow Tranlsating/Translated methods, in a similar way to what was done above for Cloning. The main difference would be that I'd rather implement these classes and interfaces in the Localization module rather than in the Framework project. Note that the IContentTranslatingPartDriver would not inherit from the IContentCloningPartDriver, otherwise we would have the same issue for the fallback that I mentioned above. How does this approach sound? I am not sure I was really able to get what I mean across, so I'd like questions and doubts about this. Even more, I'd like someone to point me to any issue I may not be seeing here.

Cheers, Matteo

MatteoPiovanelli-Laser commented 7 years ago

I'll just try to explain what I meant above when I attempted to describe the approach I would follow for the Translating/Translated. I will talk about Parts, but the same stands for Fields.

image Here I just put the stuff that is in the cloning branch. The changes all happened in Orchard.Framework (besides later adding the Cloning implementations to drivers in different modules)

image Here is, off the top of my head, what I would be adding in order to implement Translating/Translated, except of course a TranslatingContentContext class inheriting from ContentContextBase.

I hope this helps a bit to clarify this. Please, comment. Note however that I first need to settle on the Cloning aspects mentioned above.

Cheers, Matteo