tractorcow-farm / silverstripe-fluent

Multi-language translate module for Silverstripe, without having to manage separate site trees.
BSD 3-Clause "New" or "Revised" License
91 stars 110 forks source link

Occasional issues of translations being overwritten on save #191

Open teeli opened 8 years ago

teeli commented 8 years ago

I have a couple of websites running Silverstripe (current version 3.3.1) and Fluent (3.4.0). My users are reporting to me that sometimes translations are being overwritten on save (e.g. an English translation overwrites the original Finnish version). I haven't been able to reproduce this myself, but so many different users are reporting the same thing that it can't be a random coincidence.

Could this be a configuration issue? Could another extension cause problem? Is it possible it's a bug?

I have absolutely no idea what is going on, except it seems to be happening for real and I could use some help tracking it down.

wernerkrauss commented 8 years ago

Could be a config issue when you set the default_locale dynamically in a controller, or when you have another tab open and switch the current locale. At least i encountered such a behaviour with TranslatableDataObject module.

You should never change default_locale afaik.

teeli commented 8 years ago

I set i18n::set_locale('fi_FI'); in mysite/_config.php and here's my mysite/_config/fluent.yml

---
Name: myfluentconfig
After: '#fluentconfig'
---
Fluent:
  # Don't forget to set i18n->default_locale too!
  default_locale: fi_FI
  locales:
    - fi_FI
    - en_GB
  aliases:
    - fi_FI: 'fi'
    - en_BG: 'en'

---
Name: myfluenti18nconfig
After: '#fluenti18nconfig'
---
i18n:
  default_locale: fi_FI

Locale is never set in my controllers (only read)

tractorcow commented 8 years ago

I suggest not to do i18n::set_locale('fi_FI'); explicitly, since fluent will do that by itself.

If you are on an english language site and you have i18n::set_locale('fi_FI');, it could get confused. default_locale is the way to set it properly, as you have.

If a user is using the CMS in multiple windows, it's possible for one tab to alter the current locale of another tab, due to them both sharing the same session. Maybe recommend that to your users? Maybe there' a fix we could add in code to prevent this, but it would be hard. Maybe we should inject a hidden locale parameter to each page, to ensure you are saving the same locale the page was loaded in?

tractorcow commented 8 years ago

(e.g. an English translation overwrites the original Finnish version).

Has the inverse ever occured?

wernerkrauss commented 8 years ago

Maybe we should inject a hidden locale parameter to each page

Sounds awful and bullet proof...

teeli commented 8 years ago

Has the inverse ever occured?

Apparently yes. Both ways.

You might be right that the user is just doing "something wrong", because I haven't been able to reproduce this myself (which is why I'm so baffled by it, apparently it's happening "all the time"). I've told them to use only one admin tab at a time though. Actually if they are browsing the actual website in another tab for preview and changing the language there, could that affect it? I think it's always safe to assume that if a user can do something wrong, the user will do something wrong even if explicitly told not to.

So I removed the call to i18n:set_locale. Let's see if that helps.

tractorcow commented 8 years ago

Maybe we should inject a hidden locale parameter to each page

Probably going to silver-bullet this problem... however not sure when I'll ever have the time to implement it. ;D Anyone want to knock up a proof of concept and report back on it? You can probably just add it to updateCMSfields in FluentExtension.

teeli commented 8 years ago

As an update, I haven't had reports of this happening for my users anymore after updating my configuration. There was one particular page in the system (old page, but just a regular page with few custom fields, migrated over multiple upgrades past 1.5 years or so) where this kept happening more that others (although it was not the only one), so I deleted that page and created it from scratch with the same content and that seemed to help (for now at least).

tractorcow commented 8 years ago

So nothing happened and all is fixed? Sounds wonderful. 💃

Let's leave this issue open for a while... if you get no issues after a month or two I might just close it.

Thanks for the report. ;p

teeli commented 8 years ago

So nothing happened and all is fixed? Sounds wonderful.

Well, I didn't hear it's fixed. I just didn't hear further reports of it being broken, so.... yeah.

tractorcow commented 8 years ago

Ahah, I'm overly optimistic. :D

TomasTrzil commented 8 years ago

We have the same problem and we don't use i18n::set_locale in config.php,only default_locale in fluent.yml

It is very hard to analyze this steps that leads to this issue. My feeling is, that the problem exists on first translations, ie. first translation of English language will overwrite our original text in Czech language, but doesn't overwrites other languages.

tractorcow commented 8 years ago

@TomasTrzil which language is your default? Czech is the default right?

In case this happens, it might be worth showing me your yml config and a screenshot of the DB rows with the incorrect values. It could be that there is a de-synchronisation between the Field and Field_default_locale columns.

TomasTrzil commented 8 years ago

Well, ok, our fluent.yml is like this:

---
Name: fluentconfig
---
Fluent:
  # Don't forget to set i18n->default_locale too!
  default_locale: cs_CZ
  locales:
  # Add your locales as necessary
    - cs_CZ
    - en_US
    - pl_PL
    - ru_RU
  aliases:
  # Set aliases for each locale url as necessary
    cs_CZ: 'cz'
    en_US: 'en'
    pl_PL: 'pl'
    ru_RU: 'ru'
---
Name: fluenti18nconfig
---
i18n:
  default_locale: cs_CZ
---
Name: accessoryconfig
---
Accessory:
  extensions:
    - 'FluentExtension'
    - 'FluentFilteredExtension'
---
Name: closingpackageconfig
---
ClosingPackage:
  extensions:
    - 'FluentExtension'
    - 'FluentFilteredExtension'
---
Name: closingpackagecategoryconfig
---
ClosingPackageCategory:
  extensions:
    - 'FluentExtension'
    - 'FluentFilteredExtension'
---
Name: commodityconfig
---
Commodity:
  extensions:
    - 'FluentExtension'
    - 'FluentFilteredExtension'
---
Name: commoditycategoryconfig
---
CommodityCategory:
  extensions:
    - 'FluentExtension'
    - 'FluentFilteredExtension'
---
Name: branchconfig
---
Branch:
  extensions:
    - 'FluentExtension'
    - 'FluentFilteredExtension'
---
Name: didyouknowconfig
---
DidYouKnow:
  extensions:
    - 'FluentExtension'
    - 'FluentFilteredExtension'
---
Name: exhibitionconfig
---
Exhibition:
  extensions:
    - 'FluentExtension'
    - 'FluentFilteredExtension'
---
Name: packagecategoryconfig
---
PackageCategory:
  translate:
    - 'Name'
  extensions:
    - 'FluentExtension'
    - 'FluentFilteredExtension'
---
Name: machineconfig
---
Machine:
  translate:
    - 'Name'
    - 'Description'
    - 'DescriptionPerex'
    - 'Keywords'
    - 'Benefits'
  extensions:
    - 'FluentExtension'
    - 'FluentFilteredExtension'
---
Name: machinepropertyconfig
---
MachineProperty:
  extensions:
    - 'FluentExtension'
    - 'FluentFilteredExtension'
---
Name: packageconfig
---
Package:
  extensions:
    - 'FluentExtension'
    - 'FluentFilteredExtension'
---
Name: newsconfig
---
News:
  extensions:
    - 'FluentExtension'
    - 'FluentFilteredExtension'
---
Name: photoconfig
---
Photo:
  extensions:
    - 'FluentExtension'
    - 'FluentFilteredExtension'
---
Name: photogalleryconfig
---
PhotoGallery:
  extensions:
    - 'FluentExtension'
    - 'FluentFilteredExtension'
---
Name: promoslideconfig
---
PromoSlide:
  extensions:
    - 'FluentExtension'
    - 'FluentFilteredExtension'
---
Name: relationconfig
---
Relation:
  extensions:
    - 'FluentExtension'
    - 'FluentFilteredExtension'
---
Name: relationpackageclosingpackageconfig
---
RelationPackageClosingPackage:
  extensions:
    - 'FluentExtension'
    - 'FluentFilteredExtension'
---
Name: menuconfig
---
SiteTree:
  extensions:
    - 'FluentMenuExtension' 
TomasTrzil commented 8 years ago

Polish version have showed English text "Sugar and salt", when I will change Russian version to "RU Sugar and salt", Polish vetsion now shows original Czech text "Balení cukrovinek".

DB data screen is here: http://prntscr.com/b0ragy

It seems that Polish version never been saved, that is why it contains NULL. This leads me to the source of the problem...items with no regular data, just NULL, "changes" its content in admin according last saved data of other languages. If it's NULL in DB, it could show no content in Polish admin input field.

tractorcow commented 8 years ago

Ok, I've done a bit of debugging with my friend @smdevdk

It seems that if you have fluent applied twice to an object, then translations break. E.g. FluentSiteTree on SiteTree, and FluentExtension on DataObject.

I'll add a rule to FluentValidatorTask to catch this. :)

Can you do Debug::dump(Object::get_extensions('SiteTree')) and tell me what extensions you have?

phptek commented 8 years ago

Ooooh interesting.

I've also had clients reporting the exact same issue as @teeli. Everything from: "Help! 3/4 of our French translations have disappeared" to "Help! several of our Korean Translations are no longer showing."

I've yet to see what "disappeared" actually means, but have confirmed the actual disappearance of the Korean text.

This is also Fluent v3.4 on CWP v1.2 (framework / cms 3.1, PHP 5.6)

phptek commented 8 years ago

@tractorcow ...and I'm suspecting a fix to #204 may help here, the two seem similar. ("Seem"....meh)

phptek commented 8 years ago

I've done some more digging and while there may still be a bug it may be an odd one in that what it does is direct or mis identify the current locale to CMS editors, making them think they're in a different locale than they are, and causing content-editing issues.

What I suspect is going on with our users is their use of the browser's "Back" button and what they see as displayed in the CMS' LHS locale selection menu.

Under a scenario I can reproduce, but with some difficulty, the actual value of the FluentLocale_CMS cookie will differ from what the LHS locale menu is actually displaying, therefore apparently fooling users as to what locale they're really in and causing at best only confusion and at worst, content-editing mistakes such as those we see here.

Reloading the browser will show the correct locale (as per the cookie value) in the menu again, but in that prior "limbo" state users see the menu says e.g. "English (New Zealand)" but the actual locale as per the CMS cookie is something different, a prior-selected locale e.g. "Japan (Japanese)" with Japanese content showing in GridFields, site tree etc.

Once I have some rock-solid repro steps, I'll post them.

phptek commented 8 years ago

Reproduction steps with Fluent v3.4 & Framework + CMS v3.2 on CWP v1.2 (PHP 5.6 Sury and Firefox 40.1 on Ubuntu 14.04:

[OK] Login to the CMS
[OK] Select "English (New Zealand)" from the LHS CMS locale menu
...as far as CMS author is concerned, the system is operating in "en_NZ" mode.
[OK] FluentLocale_CMS cookie shows "en_NZ"
[OK] LHS Locale menu shows "English (New Zealand)"
[OK] CMS URL has correct `Fluent::query_param` (Configured as 'content_locale') set to "en_NZ"
[OK] Select any page from the CMS' site tree e.g "Home"
[OK] Select the "Main Content" tab
[??] Observe CMS URL is currently missing the `Fluent::query_param` (Configured as 'content_locale')
[OK] Select "Japan (Japanese)" from LHS CMS locale menu
...as far as CMS author is concerned, the system is operating in "ja_JP" mode.
[OK] FluentLocale_CMS cookie shows "ja_JP"
[OK] LHS Locale menu shows "Japan (Japanese)"
[OK] CMS URL has correct `Fluent::query_param` (Configured as 'content_locale') set to "ja_JP"
[OK] Select "English (New Zealand)" again from LHS CMS locale menu
...as far as CMS author is concerned, the system is operating in "en_NZ" mode.
[OK] FluentLocale_CMS cookie shows "en_NZ"
[OK] LHS Locale menu shows "English (New Zealand)"
[OK] CMS URL has correct `Fluent::query_param` (Configured as 'content_locale') set to "en_NZ"
[OK] Now press the browser's "Back" button
...as far as CMS author is concerned, the system SHOULD STILL BE RUNNING IN EN_NZ but it isn't...
[FAIL] FluentLocale_CMS cookie shows "en_NZ" (Actually shows "ja_JP")
[OK] LHS Locale menu shows "English (New Zealand)"
[FAIL] CMS URL has correct `Fluent::query_param` (Configured as 'content_locale') set to "en_NZ" (It actually shows "ja_JP")
[OK] Make an edit to the "Title" field
[OK] Press "Save"
...as far as CMS author is concerned, the system is still operating in "en_NZ" mode but it isn't
[FAIL] FluentLocale_CMS cookie shows "en_NZ" (Still shows "ja_JP")
[OK] LHS Locale menu shows "English (New Zealand)"
[FAIL] CMS URL has correct `Fluent::query_param` (Configured as 'content_locale') set to "en_NZ" (It actually shows "ja_JP")
[OK] Reload the page
...as far as CMS author is concerned, the system should still be operating in "en_NZ" mode, but can see now that it actually isn't and what's worse he/she didn't change the locale themselves.
[FAIL] FluentLocale_CMS cookie shows "en_NZ"  (Still shows "ja_JP")
[FAIL] LHS Locale menu shows "English (New Zealand)" (Actually now shows "Japanese (Japan)")
[FAIL] CMS URL has correct `Fluent::query_param` (Configured as 'content_locale') set to "en_NZ" (Still shows "ja_JP")

What this means is that CMS editors are seeing one locale in the LHS menu, but actually editing in another, causing all sorts of undefined behaviour in the system.

phptek commented 8 years ago

While the above is def a bug, I'm now not so convinced it's strictly related to #191 so I'll report it separately and refer to #191 from it.

phptek commented 8 years ago

@TomasTrzil So I've just found on the setup I'm using (New Zealand CWP) that I see this bug and that some other module I'm using does set i18n::set_locale(). Have you grepped your entire project (Not just the "mysite") directory for instances of i18n::set_locale?

tractorcow commented 8 years ago

A slightly easier to fix issue might be (for the time being) the issue of the dropdown not having the correct selected locale. Perhaps this could raise an error in the browser (in dev mode) and have this update the dropdown to the actual locale. That way you never end up in the situation where you think you are in one mode when you aren't actually.

phptek commented 8 years ago

Perhaps this could raise an error in the browser (in dev mode)

Why the dev mode restriction? The issues being encountered would seem to be occurring in PROD env's (mine certainly is).

So some sort of JS dialogue ala:

"You've clicked the browser's 'Back' button which will change your locale to ar_EG (Arabic). Is this OK?"

Selecting "Cancel" or "No" ensures the current locale is maintained, and selecting "Yes" changes the value of the CMS locale cookie as happens by default right now?

tractorcow commented 8 years ago

Why the dev mode restriction? The issues being encountered would seem to be occurring in PROD env's (mine certainly is).

Because we don't want debugging errors surfacing to the end user. In prod the best behaviour is to have a logger for these kinds of errors, but in dev it's better to just emit it immediately to the user.

TomasTrzil commented 7 years ago

@tractorcow I have tried to upgrade SS to latest 3.4.1 and also fluent to latest 3.7.0 to be sure, this in not connected with obsolete version of CMS/fluent. Problem still exists. I have tried to do Debug::dump(Object::get_extensions('SiteTree')) with the result: Array ( [0] => CyrillicURLSegmentExtension [1] => FluentMenuExtension [2] => FluentSiteTree [3] => Hierarchy [4] => Versioned [5] => SiteTreeLinkTracking [6] => UrlSegmentExtension )

We have class Page that extends SiteTree, custom field Perex saves OK in database even translated ones (see http://prntscr.com/d9rjyp), but WYSIWYG in admin shows original text, even in other Languages versions (http://prntscr.com/d9rkmq). Interesting part is, that language identifier shows that content is modified from default language (which it is - in database data), but in WYSIWYG shows the same content as in default language and it is different from what is stored in database.

tractorcow commented 7 years ago

At least that seems to eliminate the risk that you have the extension added multiple times...

If what is shown in the editor is different to what's in the database, then it points to an issue with FluentExtension::augmentSQL(). For whatever reason, maybe it always thinks it's in the default locale, even when it's not?

TomasTrzil commented 7 years ago

May this http://prntscr.com/d9rutz reffer to the fact, that the extension is used twice? How come, that all default inputs are translating ok then?

tractorcow commented 7 years ago

Yes... that would be the case.

Can you please check your project to see if it's being applied somewhere else?

You could have the extension on Page separately to SiteTree? Check Debug::dump(Object::get_extensions('Page')) maybe.

tractorcow commented 7 years ago

I've raised a side issue over at https://github.com/tractorcow/silverstripe-fluent/issues/241

TomasTrzil commented 7 years ago

It gives me the same result:

Array ( [0] => CyrillicURLSegmentExtension [1] => FluentMenuExtension [2] => FluentSiteTree [3] => Hierarchy [4] => Versioned [5] => SiteTreeLinkTracking [6] => UrlSegmentExtension )

TomasTrzil commented 7 years ago

I have investigated problem more further and found out where the problem was. It seems that it was caused by conflict of two same names "Perex" fields. One was used in News class and the other in Page class. Field was renamed and translation works like a charm now!

Thanks a lot for help, guys!

tractorcow commented 7 years ago

Oh wow, that's a real pain, but I'm glad you found the error!

tractorcow commented 7 years ago

Hello, I've built an additional validation check at https://github.com/tractorcow/silverstripe-fluent/pull/242 which will verify if you have any incorrectly applied extensions.

Can anyone with unresolved issues please check out this branch and see if their environments have any invalid configuration errors?

Run this in your CLI.

./framework/sake dev/tasks/FluentValidateTask 'flush=all'

Or go to this url in your browser.

http://localhost/dev/tasks/FluentValidateTask

tractorcow commented 7 years ago

I've now merged https://github.com/tractorcow/silverstripe-fluent/pull/242; Please update via composer and check. :D

predragffwd commented 7 years ago

Had the same issue, every time I try to change content from the non default language it get back to the default one. Also, after running Debug::dump(Object::get_extensions('SiteTree')) I got only one Fluent extension. My issue was resolved after I removed FluentExtension from SiteTree in my own config file, since it was already added by Fluent inside their extensions.yaml. Btw, to mention that I had FluentFilteredExtension on Page as well.

tractorcow commented 7 years ago

SiteTree gets the FluentSiteTree extensios, which is a customisation of FluentExtension. Can you show me the output of your get_extensions() call please?

lerni commented 6 years ago

Similar but different: I'm under the impression, BetterButtons (next/previous) can cause overwriting other languages but haven't nailed it so far.

sanderha commented 6 years ago

Some extra info regarding this issue: In the docs it says to extend FluentExtension if you have some fields on a DataExtension you want to have translated. However if I create a custom extension like this, the issue discussed in this thread occurs permanently.

The FluentValidateTask told me about this error. Sometimes I do need to translate some fields added through an extension, how can I do this?

tractorcow commented 6 years ago

Can you link the error you received?

It's possibly due to you having the extension added twice; Perhaps once through default, and another through your custom subclass.

sanderha commented 6 years ago

Indeed the issue was caused by SiteTreehaving the FluentExtension by default, and my custom subclass extension, both added to my custom page type. I thought this was the correct way to add translated fields via an extension?