I have come across the situation that the tnid of a translation node is always set to 0 instead of the source node's nid.

I have the content type set up with Workflow and Trigger modules such as that when a workflow state is set to "approved", this will trigger the action to set the node to be "published".

I have verified that the translation module did set the tnid to the source id when I first submitted the new translation node. But immediately, the tnid value is set to zero by another module and I suspect this is by the Trigger module.

By examining the function translation_nodeapi() of the Translation module. I noticed that in the "insert" case, it will update the tnid of the new node in the database. However, if another module (and in this case, the Trigger module) makes use of the hook_nodeapi(), this module will not see the new tnid value and may override the database value.

I think whenever the translation module updates some values of the node in the database, it should update the memory values so other module will see those new values.

CommentFileSizeAuthor
#21 357785_tnid_save_D6.patch758 bytesmiro_dietiker
#16 translation-357785-16.patch3.27 KBplach
#16 translation-357785-16-test.patch2.71 KBplach
#14 translation-357785-14.patch2.56 KBplach
#14 translation-357785-14-test.patch2 KBplach
#12 translation-357785-12.patch2.56 KBplach
#9 translation-357785-9.patch2.76 KBplach
#4 357785_tnid_save.patch523 bytesmiro_dietiker
#7 357785_tnid_save_D7.patch389 bytesmiro_dietiker
#5 357785_tnid_save_D7.patch349 bytesmiro_dietiker
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mdupont’s picture

mdupont
Primary language French
Location Switzerland
CreditAttribution: mdupont commented

Subscribing. This issue seems not so minor to me.

rvalery’s picture

rvalery CreditAttribution: rvalery commented

I came accross this bug by creating a new module, and overriding nodeapi hook. I'm quite new to Drupal so I don't really know if it's the right way to do it, but it seems to work for me now.

function moduleMenu_nodeapi (&$node, $op, $teaser, $page) {
	switch ($op) {
		case 'insert':
			drupal_set_message ( print_r($node, true) ) ;
			if ( isset($node->translation_source->nid) ) {
				$tnid = $node->translation_source->nid ;
			} else {
				$tnid = $node->nid ;
			}
			$result = db_query("UPDATE {node} SET tnid = %d WHERE nid = %d", $tnid, $node->nid);
			if ( $result ) {
				drupal_set_message ("Translation update was done successfully.") ;
			} else {
				drupal_set_message ("An error occured during translation update.") ;
			}
		break ;
	}
}
arnoldc’s picture

arnoldc CreditAttribution: arnoldc commented

Yes, I have a similar code in my module (the existence of it is just to work around this issue) to work around the problem. A quick glaze, the only different is that I also set the translate field to be zero. It has been a while and I forgot why? lol

I also set the weight of my module to be 1 so it will be executed later than Trigger module which has a weight of zero.

function parasun_translate_nodeapi(&$node, $op, $teaser, $page) {
  // Only act if we are dealing with a content type supporting translations.
  if (!translation_supported_type($node->type)) {
    return;
  }

  switch ($op) {
    case 'prepare':
      break;

    case 'insert':
      if (!empty($node->translation_source)) {
        $tnid = $node->translation_source->nid;       
        db_query("UPDATE {node} SET tnid = %d, translate = %d WHERE nid = %d", $tnid, 0, $node->nid);      
      }
      break;

    case 'update':
    case 'delete':
      break;
  }
}
miro_dietiker’s picture

miro_dietiker
Location Switzerland
CreditAttribution: miro_dietiker commented
Version: 7.x-dev » 6.8
FileSize
357785_tnid_save.patch523 bytes

This is really ugly. If $node finally leads to resave within the same refresh, tnid is set to 0 again after being set.
I think translation.module at least should update the $node object to represent the current state. It seems so obvious... and possibly should be considered a core bug.

Can someone please check this bug in D7? Is there a D7 port needed to make this into core?

Meanwhile a custom module might override $node->tnid to make persistency work this way as a workaround:

// tnidpreserve.module
function tnidpreserve_nodeapi(&$node, $op, $teaser, $page) {
  switch ($op) {
    case 'insert':
      if (!empty($node->translation_source)) {
        $node->tnid = $node->translation_source->tnid;
      }
      break;
  }
}

(Please note translation_source->tnid instead of nid which is wrong if you translate from non-tnid as source)

miro_dietiker’s picture

miro_dietiker
Location Switzerland
CreditAttribution: miro_dietiker commented
Title: tnid value update may be overrided by other modules » tnid value lost in case of node resave
Version: 6.8 » 7.x-dev
Priority: Minor » Normal
Status: Active » Needs review
FileSize
357785_tnid_save_D7.patch349 bytes

Checked D7 and still found this issue to be present based on code.

Attached a simple fix to make tnid persistent after save.

Can someone please make a test to bring this in?

Status: Needs review » Needs work

The last submitted patch, 357785_tnid_save_D7.patch, failed testing.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
CreditAttribution: miro_dietiker commented
Status: Needs work » Needs review
FileSize
357785_tnid_save_D7.patch389 bytes

retrying patch from drupal root..

Minoo’s picture

Minoo CreditAttribution: Minoo commented
Version: 6.8 » 7.x-dev

#4: 357785_tnid_save.patch queued for re-testing.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
FileSize
translation-357785-9.patch2.76 KB

The patch in #7 works as expected and fixes the issue, the attached one adds a simpletest that proves it. This should be ready to go now. Please confirm and set this RTBC.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
CreditAttribution: miro_dietiker commented

Looks great. Thanks for adding the test.
Didn't test the simpletest in real till now, but the code / app logic seems fine.
Someone else please review and set it RTBC to make it pass.

Status: Needs review » Needs work

The last submitted patch, translation-357785-9.patch, failed testing.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Status: Needs work » Needs review
FileSize
translation-357785-12.patch2.56 KB

rerolled from Eclipse (i.e. without the -p options)

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Issue tags: +Quick fix
plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
FileSize
translation-357785-14-test.patch2 KB
translation-357785-14.patch2.56 KB

Here is a reroll agains the current head. Attached there is also the test-only patch which shows where the bug is and therefore is supposed to fail tests.

Status: Needs review » Needs work

The last submitted patch, translation-357785-14-test.patch, failed testing.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
FileSize
translation-357785-16-test.patch2.71 KB
translation-357785-16.patch3.27 KB

Rerolled to match a change in the test function. Again the test-only version is supposed to fail.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, translation-357785-16-test.patch, failed testing.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Status: Needs work » Reviewed & tested by the community

Since I reviewed and tested the patch hunk that fixes the issue (which has been originally provided by @miro_dietiker), and the bot tells us that the test covers the issue correctly, I'm marking this RTBC.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Version: 7.x-dev » 6.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed to HEAD. Thanks!

Marking for back-port to D6.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
CreditAttribution: miro_dietiker commented
Status: Patch (to be ported) » Needs review
FileSize
357785_tnid_save_D6.patch758 bytes

Providing patch for D6. Currently untested.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Status: Needs review » Reviewed & tested by the community

Patch works as expected.

alexanderpas’s picture

alexanderpas CreditAttribution: alexanderpas commented
Priority: Normal » Major

confirming RTBC, and upgrading to major, as this bug simply makes the translation module "useless" when used together with Workflow, Trigger, Rules and similar modules, and actively destroys translation relations in certain circumstances when used together with those modules.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Reviewed & tested by the community » Fixed

Oh, nasty bug. Thanks for the fix, committed.

Dret’s picture

Dret CreditAttribution: Dret commented

Another modules is affected by this problem, this: http://drupal.org/node/995500#comment-3865998

The modules fix another drupal bug: a single node without translation is considered as NOT part of "translation group" with result to show always all icons in language switcher blocks even if only one is available.

It work fine in 6.19 but not in 6.20, where translation system result broken.

klonos’s picture

klonos
he/him
Primary language English
Location 90% Melbourne, Australia - 10% Larissa, Greece
CreditAttribution: klonos commented

If this major bug exists in 6.20, then a 6.21 should be released really soon IMHO. This effects all multilingual sites, so at least should be in the 'known issues' section of 6.20 in bold!.

Dret’s picture

Dret CreditAttribution: Dret commented
Status: Fixed » Active

This problem was described for so long time here: http://drupal.org/node/518364#comment-2870818

But seems to be considered a minor issue and probably will not be fixed in core.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
CreditAttribution: miro_dietiker commented
Status: Active » Fixed

I almost can't believe this breaks a module that implements nodeapi correctly.

Having a $node's tnid=0 after adding a node to a translation group must be considered a mistake. No module should have relied on this ever.
If there's something broken (newly), please describe the issue clearly here. In any other case the new tnid behaviour is as-it-should-be.

The object $node is now (after a tnid attachment/save) exactly in the same state as it is if the node was reloaded again. This is perfectly fine.

tnid for a single new node (without any translations) had a tnid=0 SINCE EVER. We didn't intend to change this behaviour ever with this issue here.
I can't follow you, why this is something like a new bug we introduced.

Reopen if you can provide more info. Switching back to fixed as we did successfully.

Status: Fixed » Closed (fixed)
Issue tags: -Quick fix

Automatically closed -- issue fixed for 2 weeks with no activity.

Issue tags: +Quick fix

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.