Do not rely on "content language" in hook_entity_presave()

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!"

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.

This time with less comedy debug.

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.

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.

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

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.

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.

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

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.

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

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.

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

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.

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.

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?

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.

@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.

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.

@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?

@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.

> 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.

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.

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

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.

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.

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

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.

Great patch indeed.

Thanks!!!

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.

@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?

@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.

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.

This patch adds just a (currently failing) test.

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.

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.

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).

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.

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

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

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

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.

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!

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.

#2849464: Add an API to set active language was committed.

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

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

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.

The interdiff.

Do not test display if $langcode is LANGUAGE_NONE.

This patch works only with the patch in #2856927: getActiveLanguage() should delegate its logic elsewhere..

Hi czigor,

We are not able to get green tests with #2856927: getActiveLanguage() should delegate its logic elsewhere. and this patch.
I've made some tests and tried many solutions.
Then I tried the solution from stefanos.petrakis in the Entity Translation thread (#2856927: getActiveLanguage() should delegate its logic elsewhere.) and update the title_entity_load() code accordingly.

I've rerolled the patch including that change and tests are green now.

Let me know what you think.

Here's the same file correctly named.

Hi,

Updated the patch to be more consistent.

Here's an updated patch.

It removes the language set mechanism from the title_entity_load() function because it's done automatically in title_entity_sync() when we pass NULL for the language parameter.

patch #70 applies cleanly against title beta8 (no other patches) but doesn't work for me.

With patch #70 , incorrect behavior:
Viewing the node:
English displays the english title
French displays the english title incorrect

Without any patches to the title module beta8
Viewing the node:
English displays the english title
French displays the french title correct

using entity_translation beta6
with these patches:

https://www.drupal.org/files/issues/
entity_translation_upgrade_result_parameter_must_be_array-2307937-1.patch
entity_translation_upgrade_drush_extension-1800158-2.patch
static_cache_for-2557429-17.patch
entity_translation-2734295-4.patch
entity_translation-pathauto_update-2743685-2_0.patch
entity_translation-content_and_entity_translation-2870524-9.patch
entity_translation_content_and_entity_translation_2870524_34.patch

am I missing a patch for entity_translation beta6 ?
or missing a patch for title module?

@joseph.olstad The patch in #65 only works if #2856927-2: getActiveLanguage() should delegate its logic elsewhere. is also applied. I think this is true for patches in #68, #69 and #70 too but @Pol needs to confirm this.

Hi czigor , patch 2 from #2856927 didn't apply with my against my other patches so I tried patch #8 from #2856927 with the patch #70 from this issue 2267251
with my version of entity_translation I got some unexpected results
I edited the french content from english interface using /en/2/edit/fr
changed the title
however I noticed that the automatically generated path was blank, I left it blank
saved, then I get
"Erreur 404 - Page non trouvée"
because the path was invalid.
however when I look at the french side, the title is in the correct language and it is as I saved it.

my issue here though is that now the path blanked out by default.

Just thought I'd provide this feedback.

I don't think I will pursue this as our content editors are bilingual anyway so they don't mind editing in the interface language of the content however it would be ideal to have this working so that content people could edit content in different languages but using their native language for the interface. I've seen this working before in the wetkit distro from what I recall their recipe of contrib modules and patches seems to work fairly well. If you test the latest wetkit distro release and like how it works, you might want to have a look at the wetkit_language project code to see their wetkit_language.make file and their wetkit_core project wetkit_core.make file to see what combination of patches that works for them. I know this functionality was working for them at one time however not sure if it is still ok. AFAIK they were using entity_translation beta5 with a bunch of patches last time I checked they were still on beta5.

Good luck.

I have fixed this bug using patch # 52.
Thank you.

Hello,
I was using #41 for some time and noticed it breaks with the latest entity_translation (7.x-1.0-beta7).
so I've watched back to this issue and tested patch #52 seems to correctly satisfy the following use cases:

• editing title on a multilingual entity form (node especially)
• programatically update entities via some feeds and custom importers

Also tried #70 in conjunction with https://www.drupal.org/files/issues/entity_translation-2856927-8-dual_se... without success when using feeds import.

thank you for your time

head tests are fixed,
#2813673: Tests broken since new permission in drupal core
re-queueing tests.
#70 expects another patch to be applied
#52 several others reporting successful results with patch 52.

I documented my results with #70, wasn't happy with that.
Have not yet tested #52 but others say it's good.

review patch #52, the others are failing tests.

Rerolled.

#52 is passing, see #52 several positive reviews. I haven't fully reviewed it though but it is passing, I requeued the tests recently

#2267251-52: Do not rely on "content language" in hook_entity_presave()

#82 works for me using title, features & uuid_features on a more complex project, #52 not.

(Not using the above mentioned patch in https://www.drupal.org/project/entity_translation/issues/2856927#comment...)

Rerolled #82 (which worked for us).