When a node translation set contains 2 items, and one of those has been deleted, the tnid of the remaining node is does not updated to zero, because the node_delete_multiple() call the hooks before delete the records from the table {node} and {node_revision}, etc.
Therefore record counting in the function translation_remove_from_set() is not accurate.
if (db_query('SELECT COUNT(*) FROM {node} WHERE tnid = :tnid', array(':tnid' => $node->tnid))->fetchField() == 1) {
// …
}
When $node->tnid is not empty, the result will be equal to 2 or higher.
Comment | File | Size | Author |
---|---|---|---|
#27 | tnid_update-1688144-27.patch | 4.4 KB | Gábor Hojtsy |
#24 | tnid_update-1688144-14.patch | 4.41 KB | chx |
#20 | 8.x-is-broken.patch | 250 bytes | penyaskito |
#14 | tnid_update-1688144-14.patch | 4.43 KB | lazysoundsystem |
#11 | tnid_update-1688144-11.patch | 4.43 KB | lazysoundsystem |
Comments
Comment #1
SweetchuckHasty fix
Comment #2
Gábor HojtsyThe comment under the query indicates the intention of the query was to include the node itself.
Comment #3
Gábor HojtsyAlso most probably applies to Drupal 8, should be fixed there first, then backported to Drupal 7 (and possibly to Drupal 6 if applies there).
Comment #4
SweetchuckSame patch for D8. Without tests :-(
Comment #5
Gábor HojtsySending for testing anyway. How come existing tests do not cover this?
Comment #6
SweetchuckStill needs work on this patch. To translate the interface strings and maybe more.
And maybe the whole test could be part of the TranslationTest::testContentTranslation().
Comment #7
Gábor HojtsyLooks generally like a good direction. I think its fine to keep it as its own test.
Test assertion messages should not be t()-ed anymore in Drupal. You should skip doing this.
I don't understand this comment at all. Also, it does not wrap properly.
What is random?
Also, complete that todo :)
Comment #8
Gábor HojtsyStill interested in working on this?
Comment #9
Gábor HojtsyRelated: #1645156: URL generation only works on port 80 when using domain based language negotation (that is about generation not detection).(Posted on the wrong issue).Comment #10
lazysoundsystem CreditAttribution: lazysoundsystem commentedFixed the comments as per #7. Appears that the @todo was already done.
Comment #11
lazysoundsystem CreditAttribution: lazysoundsystem commentedRemoved an added t() function.
Comment #12
ygerasimov CreditAttribution: ygerasimov commentedI have reviewed this patch. Lets wait for the bots tests.
Comment #14
lazysoundsystem CreditAttribution: lazysoundsystem commentedokay - that last minor change was a little too eager. Here goes, with quotes.
Comment #15
tstoecklerThe latest patch looks good. Fix makes sense and it is thoroughly tested.
Comment #16
Gábor HojtsyAdding sprint tag.
Comment #17
webchickCommitted and pushed to 8.x. Thanks!
Comment #18
Gábor HojtsyDid not have the backport tag, but need to be backported (http://drupal.org/node/1688144#comment-62426529).
Comment #19
penyaskitoLooking at #1305378-34: Tokens should use $options['langcode'] and not need a language object and #322995-62: Provide a distinct administration user interface language option test results, maybe some problem has been introduced with this commit?
Comment #20
penyaskitoA proof.
Comment #21
penyaskitoComment #23
webchickOk, confirmed this was breaking testbot, so I've rolled this patch back.
The problem (at least on localhost) was that it was still failing the final "Translation set is terminated." test introduced by this patch. Not sure if one of the other ones I committed today conflicted or what.
Comment #24
chx CreditAttribution: chx commentedTry this.
Comment #26
Gábor HojtsyIn other words (after discussion with @chx), this did not work anymore due to node_load() changes in http://drupal.org/node/1744046 (depreciated $conditions argument removed), which was committed before this one.
Brace yourself for more of such conflits as APIs change on an ever higher pace and developers find it harder and harder to keep up :/
Comment #27
Gábor HojtsyDid not apply, rerolled.
Comment #28
Gábor HojtsyBack to RTBC then. Thanks @chx for your help!
Comment #29
catchWhat about the existing data suffering from this bug? It should be able to provide an upgrade path to find the nodes that are in a translation set all of their own and mark those back to 0 no?
Comment #30
Gábor HojtsyYes, that makes sense.
Comment #31
Gábor HojtsyI don't think this makes sense to keep on the sprint, given that we target the removal of this making this patch be stripped out of Drupal 8 anyway. It was targeted at Drupal 8 purely as an application of process. If we need someone to spend more time on this (and people don't seem to be flocking here to do it), then it should not take away resources from the people who are trying to focus on meaningful D8MI contributions, which is what the D8MI and sprint tag combinations are for. Honestly at this point it is not easy to tell if this node-copy translation functionality will be removed for sure (still 87 days to go until feature freeze), so not turning this back to Drupal 7, but if that happens, this should be turned back there as being not applicable to Drupal 8. If that does not happen, we need to keep it on Drupal 8 and fix the bug there.
Removing D8MI tag as well in anticipation of the underlying functionality being removed from core.
Comment #32
maxtorete CreditAttribution: maxtorete commentedI'm working on the upgrade path.
Comment #33
Gábor HojtsyCross-post.
Comment #34
maxtorete CreditAttribution: maxtorete commentedComment #35
thekatic CreditAttribution: thekatic commentedD7 patch worked for me, but I was wondering why it's still not included in Drupal 7?
Comment #49
quietone CreditAttribution: quietone as a volunteer commentedDid some checking and this is no longer relevant for Drupal 9 but still is for Drupal 7. Moving back to the D7 issue queue.