Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
I've found misbehaviour of "Content translation: Translations" part, it joins tables by (node.tnid = node_source.tnid), while it should join by (node.tnid = node_source.nid).
It worked normal in 7.x-3.11 version, but is broken with 7.x-3.13 version.
The trouble is in file modules/translation.views.inc, line 65, it is:
'relationship field' => 'tnid',
and it must be:
'relationship field' => 'nid',
as it was in 7.x-3.11 version
Comment | File | Size | Author |
---|---|---|---|
#19 | views-content-translations-relationship-2638220-19.patch | 623 bytes | phergo |
| |||
#12 | interdiff-2638220-3-12.txt | 1.87 KB | jojonaloha |
#12 | views-content-translation-relationship-2638220-12.patch | 1.16 KB | jojonaloha |
| |||
#3 | views-content-translations-tnid-zero-2638220-1.patch | 1.72 KB | jantoine |
|
Comments
Comment #2
jantoine CreditAttribution: jantoine as a volunteer commentedThis was changed as a fix to this issue: #1940212: "Content translation: Translations" relationship incorrectly specifying translation source.
Comment #3
jantoine CreditAttribution: jantoine as a volunteer commentedI am seeing an issue as well where all nodes without any translations are being joined to one another because they all have a default tnid of '0'. The node table should not be allowed to join to itself where the tnid field is equal to zero. Attached is a patch that adds an additional condition on the join.
@Mav-im, please see if the attached patch fixes your issue.
Comment #4
jweowu CreditAttribution: jweowu commentedDrupal's notion of a translation set -- defaulting to a tnid of zero -- is quite a pain here. This would be much easier if all untranslated nodes simply had their own nid as their tnid. This is further complicated by #2367939: Translation sets should be 'removed' when they contain only one node, incidentally. (Or simplified by it, depending on your view point; but it's another inconsistency at any rate.)
Now we certainly don't want to join on arbitrary untranslated nodes; but we also need to ensure that the relationship does not fail entirely when there is only one node in the translation set. If tnid is zero, we still need to obtain the row for the original node.
e.g. If we're starting with an English node, then using the relationship to obtain all translations and filter to the translation for user's language, and it turns out that the user's language is English and the node has not been translated yet, returning no rows would be a bad thing.
The join presumably needs to be something like the following?
Comment #5
jantoine CreditAttribution: jantoine as a volunteer commented@jweowu,
You are absolutely correct and this is exactly what my patch accomplishes. It adds the condition on the 'JOIN' clause, not on the entire SQL statement, so the original node is returned unless the user specifically requires the relationship to exist, in which case an INNER join will be used instead of an OUTER JOIN.
Comment #6
jweowu CreditAttribution: jweowu commentedI've tested the patch, and I'm afraid that it exhibits the very problem I indicated.
In the following,
node_node
is all of the nodes in the same translation set asnode_field_data_field_promotion_reference
(also being a node), which we then filter to the specific translation for the language in question.The original join was:
Which is wrong (as per this issue).
With the patch, the revised join is now:
Which is also wrong. As per my previous reply, this join needs to be equivalent to the following.
Again, this is because in the scenario where there is only a single node in the would-be translation set, that node will have a tnid of zero, so your
tnid != 0
condition excludes it. If the tnid is zero, the join has to be on the node ID.The full query in my test was:
Which is from the following exported View (which I'll describe, as it won't be directly importable).
The view takes a Node ID argument (automatically obtained from the path for the page being viewed, so the argument represents the current page). There are two relationships.
The first relationship obtains node IDs from an entity reference field on the argument's node (each page may reference some 'promotion' nodes to be displayed. The precise promotion node IDs being referenced are not necessarily for the same language as the page which is referencing them, however).
The second relationship is the Translations relationship -- it's required, and it obtains the node translation for each referenced promotion in the current user's language.
We format the results as Content, specifying the Translations relationship (i.e. rendering the language-appropriate set of promotions for the current page).
(We also filter them by published status, and sort them on the delta value from the entity reference field).
So to adapt the example from my previous reply, if the current page references promotion node ID 50, and node 50 is an untranslated English promotion node, and the user's language is English, then we should be rendering that promotion (as it is in the correct language). But because its tnid is zero, no results will be found with this patch.
Comment #7
jantoine CreditAttribution: jantoine as a volunteer commented@jweowu,
All I see in your join statements are 'INNER JOIN' which means you are 'requiring' the relationship to exist. This feature is meant to filter out rows where there is no record on the right side of the join. If you don't require the relationship to exist, a 'LEFT JOIN' will be used instead and will keep all records on the left side regardless if there is a matching record on the right side! Here is the SQL output from my view with the relationship not required:
Please test with the relationship not required!
Comment #8
jweowu CreditAttribution: jweowu commentedI feel that we're talking at cross purposes.
If node 50 is an English node, and the relationship is looking for the English node in the translation set for node 50, then node 50 should be returned regardless of whether the relationship is "required" or not -- because the relationship does, in fact, exist: Node 50 is the English node in its own translation set, even if that set comprises only that single node (in which case tnid is zero).
The only way in which the "not required" option should ever come into play with Translations is if the relationship is constrained to a language for which no translation exists.
Comment #9
Mav-im CreditAttribution: Mav-im commentedHi guys,
don't forget we are talking about "Content translation: Translations".
So we have some SOURCE node as "node" and get its translations as "node_node".
The only relationship between them is when node_node.tnid = node.nid, so:
LEFT JOIN {node} node_node ON node.tnid = node_node.tnid
is absolutely incorrect here.
If Source node has NO translations and you want to get node itself in this case I use another handler:
Comment #10
Mav-im CreditAttribution: Mav-im commentedIn this case resulting query will be:
As you can see as Source node I use default site language (en) and 'und' language and translation language is "ga".
If there is no "ga" translation the second part of relation clause is used:
OR (node_node.tnid = 0 AND node.nid = node_node.nid)
where node loads itself if it has "und" language
Comment #11
Mav-im CreditAttribution: Mav-im commentedYou can not just take standard behaviour that have been there for years with huge amount of sites which relies on it and tell "Hey, it seems to be wrong, lets completely change its behaviour and see if it fits your private believes".
Instead of such "cause breaking all" approach would be better to think about how to make it clear by renaming it from "Content Translation: Translations" to something else, and then add another relationship which fits your "believes".
Sorry, if it seems to be rude, I'm just sure that the developer of such big and popular project should think twice before making such changes.
Comment #12
jojonaloha CreditAttribution: jojonaloha at Metal Toad commentedI was noticing that adding this relationship was causing every node to be joined to every other node when the relationship is not required and you select "All" from Translation options. I am using Views 7.x-3.13.
Based on @jweowu's suggested join:
I think this would be equivalent:
So the current patch is missing the
OR (original.tnid = 0 AND translation.nid = original.nid)
part. However, while looking at helping with a patch, I don't know how to reference theoriginal
alias in the formula. I've attached a patch that does what I think we want, but it is kinda ugly.If we want to remove the I think it would require adding another placeholder in SelectQuery::addJoin(). Unless there is another way to achieve the grouping that I'm not aware of?
I'm also not sure about what the behavior used to be or whether or not this is an API change, which @Mav-im brings up, and still needs to be considered.
Also, in case anybody else is using Views 7.x-3.13 and gets an exception, you need to update to the latest dev or apply the patch in #1090432-22: Allow additional field-to-field conditions with the default views_join handler
Comment #13
jantoine CreditAttribution: jantoine as a volunteer commented@jweowu,
I missed that you were talking about joining to the same node. In this case you are correct and there is a bug with my patch.
@Mav-im,
I was also surprised to see this change as it broke our site as well. I agree with you completely on #11. I think a better solution would have been to rename the original relationship to "Source translation" with a deprecated status and create a new "All translations" relationship. It's in the past now so let's focus on fixing the bug, yeah?
@jojonaloha,
I don't think your query below is equivalent to @jweowu's from his comment #4. With your query I
don't see how the tables will join ifbelieve the join would be to all nodes where the tnid is not zero including nodes from other translation sets?I don't believe there is a way to reference the left table in the join as the alias is only available for the right table. I believe using $def['left_table'] as your patch does uses the actual table name and not the alias assigned to it via views. This could break if other joins to the node table were part of the query. I think your solution to adding an alias for the left table in SelectQuery::addJoin() is correct, but I'm not sure how easy it will be to get a fix for this in.
If I'm not mistaken, we can write our own 'join' handler that would provide aliases for both sides of the join and instruct the relationship to use that join handler. I believe this is the best way forward at this point.
Comment #14
jweowu CreditAttribution: jweowu commentedMav-im: I agree with that approach. Reverting & renaming the original relationship, and adding a new one for the revised behaviour makes good sense to me.
In #1940212: "Content translation: Translations" relationship incorrectly specifying translation source it just hadn't occurred to me that only obtaining translations if you were starting with the translation set's source node could be a useful behaviour -- it had genuinely seemed like a mistake.
It's certainly bad to have broken views which wanted it to work that way (ignoring for a moment my "join on tnid=0" blunder in the would-be fix, which I had missed on account of the case being 100% mitigated in my own testing, and which caused even more breakage), so I'm on board with having distinct relationships for the distinct behaviours -- and giving them unambiguous names and descriptions!
Comment #15
jojonaloha CreditAttribution: jojonaloha at Metal Toad commentedSorry, you are right, the joins I listed in my previous comment are not equivalent. However, that was just based on @jweowu's example of how to solve it. Here are the actual joins that I'm seeing in a view using this relationship, which is not required and selecting "All" from Translation options.
Before the patch (which does join all the nodes with every other node where tnid=0):
After the patch:
So the result is actually that the extra formula is added as an AND condition to the base condition.
As for the use of
$def['left_table']
I could be wrong, but I think that is the alias based on a few lines up (line 56)$def['left_table'] = $this->table_alias;
So I think my patch still applies.
Comment #16
fabio84Patch at comment #12 works for me
Comment #17
shagren CreditAttribution: shagren commentedLatest patch (https://www.drupal.org/node/2638220#comment-10925969) works with latest dev version as expected
Comment #18
phergo CreditAttribution: phergo commentedPatch #12 doesn't work with the latest release 7.x-3.16. It works properly with the latest dev version.
Comment #19
phergo CreditAttribution: phergo at NTT DATA commentedPatch #12 doesn't work with the latest stable release 7.x-3.18. Please find attached a patch with a similar solution that works with the latest stable releases of Views.
The final query of this patch is the same as patch #12.
Comment #21
phergo CreditAttribution: phergo at NTT DATA commentedComment #22
phergo CreditAttribution: phergo at NTT DATA commentedComment #23
joanpebupe CreditAttribution: joanpebupe commentedPatch #12 worked fine for 7.x-3.15 but not with the latest stable views release 7.x-3.18.
However, patch in #19 works fine with views 7.x-3.18.
Comment #24
joanpebupe CreditAttribution: joanpebupe commentedComment #25
phergo CreditAttribution: phergo at NTT DATA commentedComment #26
joanpebupe CreditAttribution: joanpebupe commentedComment #28
DamienMcKennaCommitted. Thank you.