This is a follow-on from comment #10 in #1936942: translation_node_insert() updates the node table directly without also flushing the entity load cache.

translation_remove_from_set() removes its node argument from the node's translation set; but at present it doesn't remove the translation set itself unless it contains only a single node.

However, the count is made while the node argument is still in the set, so if there is only a single node in the set at that time, it means the node being removed from the set was not actually translated.

(Note also that hook_node_delete(), which calls this function, is invoked before the node is removed from the database; but this issue applies regardless of whether the node being removed from the set is also being deleted.)

I'm 99% certain that the intention here was that when a set is reduced to a single node -- at which point it can no longer be described as "translated" -- that node should revert to a tnid of zero (i.e. the same state that a new un-translated node has).

Therefore, when we remove a node from a set, it's the number of other nodes in the set which should determine whether or not the set itself survives: Removing a node from a set of two leaves us with two untranslated nodes, both of which should therefore have a tnid of zero.

n.b. The code for choosing a new translation node (when the node being removed is the translation source) is fine, because in that case the db_update() query has set the tnid of the node being deleted to zero before it queries for other nodes with the original tnid, so that query doesn't include the node being removed.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jweowu’s picture

jweowu’s picture

FileSize
1.5 KB

A re-roll with clearer comments.

webkomplize’s picture

Stumbled on this issue while getting an sql error after upgrading to Drupal 7.34 and saving an existing node which
was translated but all translated nodes deleted.

The following procedure will thrown an error:

  1. Add a new node
  2. Translate this node to another language
  3. On the source node you see now the "Flag translations as outdated" button, which is correct
  4. Delete all translations of your new node
  5. The source node still shows the "Flag translations as outdated" button, which is not correct.
    Also in database the tnid is still the same as the nid. But the tnid should be 0 ?!
  6. If you tick the "Flag translations as outdated" button now on the source node an click save you will get an sql error like this.
PDOException: SQLSTATE[42601]: Syntax error: 7 ERROR: syntax error at or near ")" LINE 2: WHERE (nid IN ()) ^: UPDATE node SET translate=:db_update_placeholder_0 WHERE (nid IN ()) ; Array ( [target] => default [return] => 2 [already_prepared] => 1 ) in translation_node_update() (Line 395 vof /xxx/modules/translation/translation.module).

This is because of the following select query in the translation_node_update function

// This is the source node, asking to mark all translations outdated.
        $translations = db_select('node', 'n')
          ->fields('n', array('nid'))
          ->condition('nid', $node->nid, '<>')
          ->condition('tnid', $node->tnid)
          ->execute()
          ->fetchCol();

which will now get no result and the following update sql statement

        db_update('node')
          ->fields(array('translate' => 1))
          ->condition('nid', $translations, 'IN')
          ->execute();

will throw this error because the nid has no value.

With the patch of this issue, this seems to be fixed for new nodes and translation deletes.
For existing standalone nodes which was translated in history but then all translations deleted before this patch, it make sense to catch the unusually (see procedure above) but possible no sql result case from the select query above.

jweowu’s picture

Agreed; better for translation_node_update not to assume the data is in a valid state.

I suspect we should also write an update hook to fix that data, removing all single-node translation sets.

legovaer’s picture

I've added a test in order to test if this Issue is fixed. The test will reproduce the steps as described in comment #3.

Status: Needs review » Needs work

The last submitted patch, 5: drupal-7.x-remove_translation_set-2367939-3.patch, failed testing.

legovaer’s picture

Status: Needs work » Needs review
FileSize
1.83 KB

This patch provides additional test coverage for the translation module. It will run following steps;

  1. Authenticate as an admin
  2. Enable the Spanish language
  3. Authenticate as a translator
  4. Create a new (English) node
  5. Create a Spanish translation of the previous created node
  6. Authenticate as an admin
  7. Delete the Spanish translation again
  8. Edit the English node and check "Flag for outdated translation"
  9. Save the node
  10. Expect "!post %title has been updated."

The current result is an exception.

Note: both roles "admin" and "translator" are not changed. Only one additional test function has been added.

Status: Needs review » Needs work

The last submitted patch, 7: drupal-7.x-remove_translation_set-2367939-6.patch, failed testing.

agileadam’s picture

Component: translation.module » ajax system

+1 for the patch in #2

This corrected this issue:

1. Create a language neutral node (locale buttons are enabled)
2. Switch node from language neutral to English
3. Add Spanish translation
4. Delete the Spanish translation
5. Edit English node; set back to language neutral
6. View page and attempt to click global locale to any other language. Buttons are disabled because translation set still exists (tnid should be 0 at this point but it's not).

With the patch, after step 5 the tnid value goes back to 0 so the node appears for all languages.

jweowu’s picture

Component: ajax system » language system

Fixing the issue component. "translation.module" was certainly the correct choice originally, but it no longer exists as an option! (which explains why "ajax system" -- the first option in the list -- ended up being chosen by default in the previous update). "language system" seems like the best current alternative.