Problem/Motivation

Currently title_entity_presave() uses title_active_language() which basically relies on the content / interface language set if not actively set otherwise.
This means it works fine for e.g. en/node/1/edit/en because the interface language is the same as the translation language.
However, it fails if de/node/1/edit/en is used even though this is a perfectly fine / allowed / reasonable path to edit a translation (without having to switch the interface language too).
The issue leads to overwritten titles in the interface language.

Proposed resolution

Instead using title_active_language() use the form language of the entity translation handler.
Another approach would have been to use the form language of the entity translation handler only if one is explicitly set, but unfortunately the entity translation handlers (currently) provide no way to check that.

Remaining tasks

See if tests pass - locally I get very random test failures and I can't figure out why (independent if the code base)
I've added a new test which uses another interface language instead the translation language as path prefix. But I don't know yet if this test is valid the way it is.

User interface changes

None.

API changes

None.

Files: 
CommentFileSizeAuthor
#65 interdiff-63-65.txt631 bytesczigor
#65 title-language-2267251-65.patch12.07 KBczigor
#52 title-do_not_rely_on_title_active_language-2267251-52.patch1.49 KBstefanos.petrakis
#51 title-do_not_rely_on_title_active_language-2267251-51.patch741 bytesstefanos.petrakis
#48 title-language-2267251-48.patch1.28 KBczigor
#41 title-language-2267251-41.patch9.93 KBczigor
#27 2267251-27.patch9.93 KBLeksat
PASSED: [[SimpleTest]]: [MySQL] 319 pass(es). View
#24 2267251-24-step_6-should_pass.patch9.57 KBLeksat
PASSED: [[SimpleTest]]: [MySQL] 313 pass(es). View
#21 title-2267251-20.patch1.99 KBseanr
FAILED: [[SimpleTest]]: [MySQL] 299 pass(es), 12 fail(s), and 17 exception(s). View
#19 title-2267251-19.patch1.44 KBseanr
FAILED: [[SimpleTest]]: [MySQL] 300 pass(es), 11 fail(s), and 6 exception(s). View
#14 title-2267251-11.patch3.12 KBcatch
PASSED: [[SimpleTest]]: [MySQL] 311 pass(es). View
#12 title-2267251-11.patch4.58 KBcatch
FAILED: [[SimpleTest]]: [MySQL] 316 pass(es), 5 fail(s), and 0 exception(s). View
#10 title-2267251-10.patch5.62 KBcatch
FAILED: [[SimpleTest]]: [MySQL] 316 pass(es), 5 fail(s), and 0 exception(s). View
title-do-not-rely-on-interface-language.patch2.49 KBdas-peter
FAILED: [[SimpleTest]]: [MySQL] 306 pass(es), 15 fail(s), and 1 exception(s). View
title-do-not-rely-on-interface-language-test-only.patch1.58 KBdas-peter
FAILED: [[SimpleTest]]: [MySQL] 316 pass(es), 5 fail(s), and 0 exception(s). View

Comments

The last submitted patch, title-do-not-rely-on-interface-language-test-only.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, title-do-not-rely-on-interface-language.patch, failed testing.

das-peter’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, title-do-not-rely-on-interface-language.patch, failed testing.

erwangel’s picture

The diffs in title.module part of patch #3 worked for me.
I had a problem while saving a translation field_title, both field_title in the original language and and legacy/drupal title were overwritten with the translated value. The other way, editing the original language title,did not affect the translated titles. So, after a translation, I had to go back to the original and set back the initial title value. Patch resolved this issue.

Also, the problematic behavior described above created some url issues if url_redirect and alias (path module) were active, because writing and rewriting the title lead to circular redirections, which resulted to a nice warning (status message) : "Oops, looks like this request tried to create an infinite loop. We do not allow such things here. We are a professional website!"

The last submitted patch, title-do-not-rely-on-interface-language-test-only.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, title-do-not-rely-on-interface-language.patch, failed testing.

catch’s picture

Priority: Normal » Critical
Status: Needs work » Needs review
FileSize
5.62 KB
FAILED: [[SimpleTest]]: [MySQL] 316 pass(es), 5 fail(s), and 0 exception(s). View

I'm bumping this to critical because it's easy via programmatic saves to end up with one of two situations:

1. The legacy field value overwritten with the value of a translation.

2. Either the translated or original foo_field fields getting overwritten in the wrong language

Here's an updated patch, fixes the non-UI test.

The test changes show the effect of this. If you're doing programmatic saves, and need the title synced, you have to set the langcode with setFormLanguage().

I think this is OK - while modules quite often save nodes programmatically, they rarely change the node title then save.

Also where modules do this, that's exactly where things can get into trouble since title_active_language() has a static cache and there could be different nodes in different languages saved in the same request.

One question is whether there's an additional backup we could do as well as the form language - could we fall back to whether a translation is added or removed and take the language from that as well? If we did that it'd mean less changes but not sure how practical that actually is.

Status: Needs review » Needs work

The last submitted patch, 10: title-2267251-10.patch, failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
4.58 KB
FAILED: [[SimpleTest]]: [MySQL] 316 pass(es), 5 fail(s), and 0 exception(s). View

This time with less comedy debug.

Status: Needs review » Needs work

The last submitted patch, 12: title-2267251-11.patch, failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
3.12 KB
PASSED: [[SimpleTest]]: [MySQL] 311 pass(es). View

New tests added in the patch don't pass, maybe that URL is no longer valid or it needs setting up more?

Uploading with that to get a green patch for now.

catch’s picture

Here's a link to the current title-specific workaround in drafty: http://cgit.drupalcode.org/drafty/tree/drafty.module?h=7.x-1.x#n166

The issue there is:

1. Node is loaded in content language

2. Draft is saved in content language.

3. As draft is saved, we then load the current active revision, then save it again as a new revision, so that it stays active. This is a 'clone' operation so there's zero changes made to the entity.

It's the third part of this operation where the current behaviour breaks without some kind of intervention.

erwangel’s picture

applied manually #14 seems to work on 7.x-1.0-alpha7+12-dev

chx’s picture

I was able to reproduce this by editing the title when adding a translation. But my setup includes cps+drafty+entity_translation so who knows. The issue fixed the problem.

kitikonti’s picture

Same issue on my site, and i think this one is really very critical. It destroyed hundreds of translations on my site. When using entity translation export import or tmgmt to import/add new translations it will overwrite the current active language of all imports with the source language.
The patch from #14 seems to solve the problem. In my case i needed to flush the cache multiple times after applying.

seanr’s picture

FileSize
1.44 KB
FAILED: [[SimpleTest]]: [MySQL] 300 pass(es), 11 fail(s), and 6 exception(s). View

Needs one more check for isEntityForm, otherwise it incorrectly catches Panelizer submissions and still breaks the title. See attached.

Status: Needs review » Needs work

The last submitted patch, 19: title-2267251-19.patch, failed testing.

seanr’s picture

FileSize
1.99 KB
FAILED: [[SimpleTest]]: [MySQL] 299 pass(es), 12 fail(s), and 17 exception(s). View

Another attempt - this is working for us on a project, but not sure about the failing tests - hopefully these pass now? Additional fix here by jgao.

seanr’s picture

Status: Needs work » Needs review

Looks like that didn't get queued for testing - does it need to be reuploaded?

Status: Needs review » Needs work

The last submitted patch, 21: title-2267251-20.patch, failed testing.

Leksat’s picture

It was not that easy :)

I created a step-by-step patches to better describe the changes.

Step 1. First I fixed the drupalPost() calls. Whatever path is passed to drupalPost() or drupalGet(), first goes to url(), so the path should not contain a path prefix, but the "language" option should be passed instead.

Step 2. I added setFormLanguage() calls from #14, so the programmed translation workflow is complete.

Step 3. There I updated test, so it fails on URLs like "de/node/1/edit/en".

Step 4. The title_active_language() function is changed completely. Now it uses the getFormLanguage() method of entity translation handler.
Also, the TitleTranslationTestCase::termLoad() is updated to not use title_active_language().

Step 5. Tests were happy, but after manual testing I noticed that legacy fields are not synchronised anymore, so I extended tests to check that too.

Step 6. The title_active_language() is updated to decide if it needs to use form language or content language.

Leksat’s picture

Hey! Why only the last patch was tested? I thought all of them should be queued. That's not fair...

Leksat’s picture

Assigned: Unassigned » Leksat
Status: Needs review » Needs work

Manual testing showed that it behaves incorrectly when editing source translation on a non-source language prefixed path. I'll extend tests to handle this case too.

Leksat’s picture

Assigned: Leksat » Unassigned
Status: Needs work » Needs review
FileSize
4.55 KB
7.61 KB
9.93 KB
PASSED: [[SimpleTest]]: [MySQL] 319 pass(es). View

Okay. I removed the title_active_language() completely to make the code more clear.

Now title_entity_presave() always passes $handler->getFormLanguage() to the title_entity_sync().

When language is not passed to the title_entity_sync(), it uses:
- set: title original language
- get: content language
So, yes, we still rely on the "content language", but only when performing "get" synchronization.

Also updated test to check the case described in #26.

Leksat’s picture

I have issues with my #27 patch. Even if tests pass, manual testing shows that in programmatic translation workflow, the value of $entity->title is synchronized to the current content language translation. Which is not good.

Anyway, if someone explain me how the title module should work, I'll be able to fix this.

How the module should work in my opinion:
On entity load: set $entity->title from $entity->title_field[{current-content-langcode}][0]['value'] (so the $entity->title is displayed correctly on the page, etc)
On entity save: set $entity->title from $entity->title_field[{entity-original-language}][0]['value'] (so, the $entity->title is always comes from one language and does not change over time)
When updating bundle to entity_field: populate all translations of $entity->title_field from $entity->title

That is all I expect from the module. But, actually, tests are written to expect other things. Can someone specify the desired module behavior?

kitikonti’s picture

I cant get this working, which is really annoying. I have a live site where i need translations all the time. I use tmgmt and local translators to translate my nodes. But i have not found any combination of module version/s or patches to get this working. Actually my client is really angry and i don't know what to do. Nobody knows how this module should work, or does not know why it dont work.
In my current case, the source language of my nodes gets overwritten with the sites default language if a translator is using his nativ language while saving the translation.
So i have a french translator, a node in german only (source) and the sites default language is englisch. So i translate the node to english first. Then the french translator using the site in french language, translates the node to french. When he saves the translation the source language (german) will be overwritten with the englisch (default) title.

Leksat’s picture

@kitikonti, can you try https://github.com/AmazeeLabs/title/tree/7.x-1.x-amazee ?
Today I disabled reverse synchronization (see README.md), and manual testing showed that everything works as expected now, including tmgmt translation.

kitikonti’s picture

And as a notice, this is not a problem of my complex site configuration. I have also a completely new installation with the minimum configuration where i get the exact same behavior.

kitikonti’s picture

@Leksat Thx for this, in my clean install it already works. Testing on my live example will take a while. But what is the downside of this version? I am a bit scared to use this special version when i read:

There are bugfixes we need that are really special, so they won't be committed to contrib module.

And what means this reverse sync? What does it do and when do i need it?

Leksat’s picture

@kitikonti,
> I am a bit scared to use this special version
You can find a list of actual changes here: https://github.com/AmazeeLabs/title/compare/7.x-1.x...7.x-1.x-amazee (see commit messages, they mostly point to d.o issues)
> And what means this reverse sync? What does it do
Reverse synchronization copies value of legacy field to regular field. For example from $entity->title to $entity->title_field[$langcode][0]['value'].
> and when do i need it?
Personally, I don't know cases when this can be useful.

Soul88’s picture

> Personally, I don't know cases when this can be useful.
It is useful when you try to save translation of a node from the wrong language.

kitikonti’s picture

Is this the only case where you need this or only one of many? Because if it's the only one couldn't we just add a condition for this single case? And if we do so, will this case also conflict with the current problem or not?

I am really wondering why not more people have this problems, because they are existing in a completely new installation. The title module itself has nearly 50000 reported installs, and if the problem will only be triggered from another contrib module like in my case tmgmt there are still more than 2000 reported installations. Non of these could really work? Thats strange. I hope they didn't merged these problems in d8 core.

Leksat’s picture

Btw, @kitikonti, which title module version you used when you experienced problems you described in #29?

erwangel’s picture

7.x-1.0-alpha7+14-dev (2015-mar-23) still overwrites the translated title_field by title_field's value in the original language after editing/saving the original language. Once again fix #14 was sufficient for this problem, but the legacy node->title is not updated while editing the original language.

leksat wrote:

On entity load: set $entity->title from $entity->title_field[{current-content-langcode}][0]['value'] (so the $entity->title is displayed correctly on the page, etc)
On entity save: set $entity->title from $entity->title_field[{entity-original-language}][0]['value'] (so, the $entity->title is always comes from one language and does not change over time)
When updating bundle to entity_field: populate all translations of $entity->title_field from $entity->title

I think the workflow described here is the good one but we have to distinguish entity load for displaying the node from loading it for editing purposes.
$entity->title should not change over time, it should change only when title_field changes in the original language. Perhaps, while in a translation editing form, the entity->title can be printed/reminded on top of the form or above the title_field through hook_form_alter, so translators can see what the original title is and this way avoid confusion.

das-peter’s picture

@Leksat Thank you so much for your work. I'll give the AmazeeLabs fork right now.
As for "There are bugfixes we need that are really special, so they won't be committed to contrib module." - I wouldn't agree with that ;)
I checked out https://github.com/AmazeeLabs/title/compare/7.x-1.x...7.x-1.x-amazee and I think even the disabled reverse synchronisation would go in - if made configurable.
Instead of removing it completely I'd add a condition that checks for a variable - by default it passes. But everyone can disable it.
Something like this:

if ($langcode && variable_get('title_module_enable_reverse_sync', TRUE)) {
    title_entity_sync($entity_type, $entity, $langcode, TRUE);
}

Add an uninstall hook with variable_del() and add a readme section about the option. And voilà, I'd say absolute compatible to contrib :) Over time we might switch to disabled by default if it turns out to be the right way ;)

Regarding this:

That is all I expect from the module. But, actually, tests are written to expect other things. Can someone specify the desired module behavior?

Can you point out a particular test?
As far as I know you pretty much described the intended behaviour on entity load / save handling. But it seems the module also tries to take care of some display aspects e.g. title_process_page().

das-peter’s picture

@Leksat FYI: The disabled reverse sync indeed seems to fix an issue I had with inline editing of Commerce Products in Product-Display Nodes when using entity translation. I'd say we should go for an official patch ;)

Further thoughts on:

Even if tests pass, manual testing shows that in programmatic translation workflow, the value of $entity->title is synchronized to the current content language translation. Which is not good.

and

On entity save: set $entity->title from $entity->title_field[{entity-original-language}]

I think this is a point where a lot of confusion comes up.
In a programmatic translation workflow:

  • There isn't automatically an entity form language available - so it falls back to the entity language.
  • We might don't know what was used to store the "new" title; the title_field or title property - so which value should go where? ;)

Disclaimer: That's just what I remember and always bothered me when handling programmatic translation or imports(!) I would have to take a closer look to make sure I'm up to date.
What could be especially difficult for imports is that you might have an incomplete entity which means you won't have title_field[{entity-original-language}], and thus nothing to sync back to the legacy property.
Arrrgh, still lot's of edge cases - I think the only way to handle this is to enhance the documentation ;) Not sure if we can cover all the things in code :|

james.williams’s picture

Status: Needs review » Reviewed & tested by the community

Patch from #27 has been working for us, and in the wild on client sites, for a good while now. It may not fix everything (e.g. when doing things in a programmatic workflow, outside the context of a form handler), but those cases could be caught in follow-up issues, if they are even worth supporting. I suspect that the cases described in comments #28 onwards are best supported by documentation anyway, given they are based on doing something through code, outside the UI?

I'm going to mark this as RTBC in an effort to propel this forward - feel free to disagree and undo that if anyone thinks it's worth holding back. Even if it could be improved further with follow-ups, I do believe the patch from #27 would still be worth committing given that it works on large sites in the wild, satisfies existing tests, and will fix the issue for anyone not using custom code to do things programmatically.

czigor’s picture

Patch #27 works for me and also makes sense. Just a reroll.

hgoto’s picture

Thank you for working on this. Patch #27 works for me. I agree that this can be committed.

I tested manual tests in the following cases. (I prepared 3 languages for the tests: en(English), langA, langB. Here, the source language is English.)

  • Node title edit
    • editing title in langA with content language en: OK (OK means that the source title is not overwritten.).
    • editing title in langA with content language langA: OK.
    • editing title in langA with content language langB: OK.
    • editing title in en with content language en: -
    • editing title in en with content language langA: OK (OK means that the other translated title is not overwritten.).
    • editing title in en with content language langB: OK.
  • Term name edit ... I tested the same pattern as the node title edit.
Pol’s picture

Great patch indeed.

Thanks!!!

czigor’s picture

Status: Reviewed & tested by the community » Needs work

The patch #27 (and #41) breaks the module in the following scenario:
0. Fresh drupal 7 install with title, entity_translation and devel modules enabled, all code is latest dev.
1. I have a node (nid = 1) with 'en' and 'de' title_field 'Title EN' and 'Title DE' resp. The node language is 'en'.
2. I run this code on /de/devel/php:

$n = node_load(1);
node_save($n);

If the patch is applied, the 'en' title_field gets overwritten with 'Title DE'.

If there's no patch applied or we run the code from /en/devel/php, then the 'en' title_field stays 'Title EN' as it should.

james.williams’s picture

@czigor -- I really don't think that delaying committing this patch in order to support working outside of the context of a form is worth doing, this is a critical issue causing data loss (overwriting node titles).
Especially as the issue is just being seen when doing it programmatically from /devel/php, which that doesn't truly replicate 'real' scenarios, which can almost always include the 'extra' code needed to work correctly - i.e. to set up a translation handler so that any code can be aware of what needs to happen. That's pretty much the point of the patch!

Surely the patch from #27 should be committed, as it has been confirmed by several people and has been working for over a year on many production sites. Then a follow-up issue could be created to deal with saving nodes outside of the context of forms.

I won't put this back as RTBC myself, as maybe I really am the only person who thinks that it's worth delaying this critical fix in order to support bespoke code, but surely not?

czigor’s picture

@james.williams The devel/php description was meant just to make it possible to easily reproduce the real-life issue I'm experiencing on my site with the patch applied. I'm not using devel/php on my production site either :).

BTW this issue with this patch has been already pointed out by das-peter in #39 by saying:

There isn't automatically an entity form language available - so it falls back to the entity language.

All in all, I don't think replacing one bug with another is the correct way to proceed but I don't know the solution either.

james.williams’s picture

Ok, understood then. Have you tried the work that has been done in #1991988: Adding a new translation overwrites the English/source field value? Maybe a combination of these two tickets is needed as the overall fix.

czigor’s picture

This patch adds just a (currently failing) test.

czigor’s picture

Status: Needs work » Needs review

I believe the original issue (translating ) is an Entity translation module one but it needs a test here, in the Title module. I have submitted a patch in #2849464: Add an API to set active language which makes all Title tests green, including the new test in #48.

czigor’s picture

For me the patches in #2849464: Add an API to set active language and #2768857: Overwritten titles when using entitycache, panelizer and entity_translation modules fix all (form and programmatic) field overwriting issues. All we need to commit here is the test in #48.

stefanos.petrakis’s picture

I think that the problem here is partly conceptual (what we expect from the machine); these edge cases point out that there is no system-wide handling of the language_code suffix in URLs, not by core and not by Entity translation in a service-able way. But this is a different issue, I see a future need for a URL suffix negotiation provider.

I looked at the suggestion/patch over at (https://www.drupal.org/node/2849464) and I responded with a patch that delivers a hook for notifying about incoming content form languages. This works only on the add/edit entity translation forms at the moment. But, it's an improvement and would allow some future implementations to expand on it.

I attach here a small patch with an implementation of that hook, which passes all the tests, incl. the one provided in this issue (very thankful for it since they cover the contradicting language codes in the URL).

stefanos.petrakis’s picture

Forget this last comment!

I think there is an even cleaner and easier solution in getting the active language:
by looking into the $translations->hook array property. And this is following what is already happening in line 145 with the check for the 'delete' hook.
This is the most reliable way to get the active language for a given entity translation, by 'asking' the hook that is going to take place what language it's targeting.

Here is a patch that:

a) Keeps everything that is important inside the Title's own code.
b) Utilizes the language negotiation logic from Entity translation by checking the $translations->hook property, as already done for the case of 'delete' hooks.
c) Passes all the tests, including the ones from #48

There is still one test missing: testing the 'insert' hook (translation add), but I am not sure if this is actually needed as I am missing the use case.

czigor’s picture

I still think that it's each module's (i.e. entity translation's) job to set the active language for its own routes.

stefanos.petrakis’s picture

I don't necessarily disagree, I think the language negotiation logic for URL suffixes should be exposed on a system-wide level.

Till that happens, the Title module should - for it's own sake - stick to being 'smart' about Entity translation's URLs, the way it has already been doing in the case of a 'delete' translation hook inside entity_presave.

I also don't think that calling title_active_language() from ET is the same as ET setting the active language for its own routes. In that case, ET would be expected to call title_active_language(), module_a_active_language(), module_z_active_language(), etc. with every module that needs that information. Where a hook (like suggested in #51) would be more appropriate.

I am however still confident about the patch in #52 providing a good way to solve this, and we need to figure out what stands against:

  • solving a data-integrity bug of the Title module
  • inside the module's own code base
  • following ideas that are already part of the module's own code-base
  • without affecting other modules
plach’s picture

We're discussing a possible approach to finally fix this at #2849464: Add an API to set active language. Feedback welcome :)

czigor’s picture

Discussed this in #2849464: Add an API to set active language and I think the conclusion is very close to #27. Attached a very similar patch, which only adds an additional getFormLanguage() call in title_entity_sync(). This fixes my issue described in #44.

stefanos.petrakis’s picture

Status: Needs review » Reviewed & tested by the community

I checked the code (incl. the rewriting that took place in the test).
Looks good and the tests are green (when combined with the latest patch from #2849464-26: Add an API to set active language).

Thanks!

czigor’s picture

Status: Reviewed & tested by the community » Postponed

Let's wait until #2849464: Add an API to set active language gets in, we will have to replace get/setFormLanguage() with get/setActiveLanguage() then.

plach’s picture

Status: Postponed » Needs work
czigor’s picture

Status: Needs work » Needs review
FileSize
11.66 KB

Updated #56 and replaced all ?etFormLanguage() with ?etActiveLanguage().

plach’s picture

Status: Needs review » Needs work

Thanks, I will have a close look to this ASAP.

plach’s picture

Status: Needs work » Needs review
czigor’s picture

Status: Needs review » Needs work
FileSize
12.02 KB

The patch in #60 fixes the translations saving but breaks the display. Added a test to prove this.

That is, when loading an entity when the global $language is 'en' and the entity original language is 'it', the legacy field will be in 'it'. This is because there was no setActiveLanguage('en') called and thus the active language falls back to the entity original language.

czigor’s picture

FileSize
406 bytes

The interdiff.

czigor’s picture

czigor’s picture