Skip to content

Localization#7596

Merged
sebastienros merged 8 commits intoOrchardCMS:devfrom
LaserSrl:OrchardCMS/devLocalization
Apr 3, 2017
Merged

Localization#7596
sebastienros merged 8 commits intoOrchardCMS:devfrom
LaserSrl:OrchardCMS/devLocalization

Conversation

@MatteoPiovanelli
Copy link
Copy Markdown
Contributor

This PR contains the things we did for Localization.

  • Updates to handling of MasterContentItem (as they are in 1.10.x)
  • Synchronization of CultureNeutral Fields and Parts (based on Cloning)
  • Support for multi-language Blogs (in its own feature in the Blogs module)
  • Support for multi-language Taxonomies (in its own feature in the Taxonomies module)

cc:
@sebastienros @HermesSbicego-Laser @BenedekFarkas @GiuseppeMusso-Laser @LorenzoFrediani-Laser

I brought the updates to the localization module done in 1.10.x that fixed issues with the MasterContentItem and the Synchronization of CultureNeutral parts/fields
Brought the extension feature for Blogs' localization.
I brought in the code for Taxonomies, Terms, TaxonomyFields
@sebastienros
Copy link
Copy Markdown
Member

Thanks. Again we'll need someone else to try it.

@BenedekFarkas BenedekFarkas self-requested a review March 2, 2017 21:13
@MatteoPiovanelli
Copy link
Copy Markdown
Contributor Author

Has anyone had the chance to test this?

@BenedekFarkas
Copy link
Copy Markdown
Member

It's in my pipeline!

@MatteoPiovanelli
Copy link
Copy Markdown
Contributor Author

Ok. Cool.
Let me know if anything is unclear and/or messed up.

@sebastienros
Copy link
Copy Markdown
Member

Is there anyone else we could ask to try this branch?
Maybe @urbanit ? @Xceno ? @jersiovic

@urbanit
Copy link
Copy Markdown
Contributor

urbanit commented Mar 16, 2017

Ok, the scenario that should be tested is:
create multilingual taxonomy, add them to multilingual blogpost which has some culture neutral fields as well as some culture specific fields. Publish one language, update another etc. Right?

@HermesSbicego-Laser
Copy link
Copy Markdown
Contributor

Yes, the scenario you described cover functionality quite well, so you could start testing that one. Other combinations are possible but are less concrete.

@Xceno
Copy link
Copy Markdown
Contributor

Xceno commented Mar 17, 2017

@sebastienros Yep, I'll test it over the next two days

@urbanit
Copy link
Copy Markdown
Contributor

urbanit commented Mar 17, 2017

@MatteoPiovanelli-Laser great job!
Scenario tested:
contentItemLangSpecific
contentItemLangNeutral
contentItemNoLocalized
Taxonomy Localized with terms localized
Taxonomy Localized with terms neutral (a small bug here that terms should not have lang? otherwise user has the option to create translation and it fails)
Blog localized
Blog Post localized with:

  • contentItemLangSpecific
  • contentItemLangNeutral
  • contentItemNoLocalized
  • Taxonomy Localized with terms localised
  • Taxonomy Localized with terms neutral
  • fieldLangSpecific
  • fieldLangNeutral

Publishing sync as ti supposed to sync (terms, neutral ci and neutral fields),remove specific lang contents and add, when possible, the correct lang ones.

@sebastienros by far, the best localization implementation across all cms I have used (wordpress, drupal, dnn).

@sebastienros
Copy link
Copy Markdown
Member

Good job @MatteoPiovanelli-Laser !

Still giving some time for @Xceno and @jersiovic to give their opinion if they want as they participated to the meetings.

@jersiovic
Copy link
Copy Markdown
Contributor

Yeap it looks amazing what you implemented. Sorry for not answering before I was not sure I could find time for testing it. Along next week I think I could give it a try.

@Xceno
Copy link
Copy Markdown
Contributor

Xceno commented Mar 20, 2017

Hi folks!
I'm done with testing and yes, it's a huge step forward for localization.

  • I've created various content types, created and deleted translations through ui and code in all weird ways I could imagine.
  • @MatteoPiovanelli-Laser, until now there was an issue that the MasterContentItem is different for different translations, that seems to be fixed now?
  • Taxonomy localization works really well too, but I can confirm the bug @urbanit mentioned above.
    However, I think a lot of folks will appreciate the effort gone into taxonomy localization.
  • The culture neutral field and parts are a huge plus for me.

I also appreciate the comments throughout the code!

So, I'm happy with it and didn't find anything breaking apart. And since it'll be merged into dev we'll have enough time to test it some more.

@MatteoPiovanelli
Copy link
Copy Markdown
Contributor Author

Thanks everyone for testing this, and for the compliments.

@Xceno the MasterContentItem stuff actually got fixed in 1.10.x by a PR I did. Sometimes the MasterContentItem would not carry over correctly when chaining translations, and in fixing that everything slotted nicely into place.

I'll give a look at the small bug you guys mentioned.

@MatteoPiovanelli MatteoPiovanelli force-pushed the OrchardCMS/devLocalization branch from d13632d to d2bd4d0 Compare March 20, 2017 11:37
@MatteoPiovanelli
Copy link
Copy Markdown
Contributor Author

Hello.

@LorenzoFrediani-Laser and I have been checking the issue you guys mentioned.
We discovered that things were not working as we designed them, because this branch was missing the implementation of the Cloning method for TaxonomyPart.

My latest commit here fixes that.
What happens now, all taxonomies in a Localization Set share the same Term ContentType. The handler in the extension takes care of correctly matching/moving translated terms.
The Terms for taxonomies that are marked as Neutral when we create them don't have a LocalizationPart (earlier, they had it because of a default setting that would trigger because the TermTypeName was not being cloned when doing a New Translation of a taxonomy).

@sebastienros
Copy link
Copy Markdown
Member

@Xceno @urbanit last small verification the bug is fixed before I merge it? Thanks guys 🥇

… (as TryToLocalizeItems in ContentPickerField settings).
@GiuseppeMusso-Laser
Copy link
Copy Markdown
Contributor

Default settings in TaxonomyField changed according to default settings in ContentPickerField as discussed in #7632 .

@urbanit
Copy link
Copy Markdown
Contributor

urbanit commented Apr 3, 2017

Hi, I tried the fix (not the latest one) and it works. But, I do not know what's the point in a localized taxonomy to have culture neutral terms that can be selected in any language.... It 's a tricky situation... But, the bug is gone.

? _contentManager.Query<LocalizationPart>(localized.ContentItem.ContentType)
: _contentManager.Query<LocalizationPart>(versionOptions, localized.ContentItem.ContentType);
IContentQuery<LocalizationPart> query;
if (content.ContentItem.TypeDefinition.Parts.Any(x => x.PartDefinition.Name == "TermPart")) { // terms translations can be contained on different TermContentType linked to taxonomies translations
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@MatteoPiovanelli-Laser @sebastienros

I just found this during merging 1.10.x to dev and I have to admit that I looked at this PR and didn't notice this, but this is really bad. We can't put a reference like this in a module that provides low-level functionality (it's not a dependency, so it doesn't break anything, but still).

What's really strange is that two years later, @HermesSbicego-Laser committed a change that removes this, then two commits later I merged 1.10.x into dev, which readded that change. I don't know how the difftool would think that that should happen, but re-adding removed code by accident (without noticing) is even worse. This is not the first time it happens (and merging from 1.10.x to dev caused a lot of different problems in other cases too) - suggestions are welcome. I'll try to sort out the correct logic for LocalizationService...

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Update: I made a mistake, Hermes didn't remove this (I was looking at the wrong diff). Nevertheless, this TermPart-related logic has to be removed somehow without breaking Taxonomy Localization.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Removed the TermPart condition in this commit: a5a24d0, so there's no type filter in the query. This way it doesn't Taxonomies localization and I don't think filtering on content type matters anyway.

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.

10 participants