Patch for tracking translated nodes as the original

I've reviewed this patch, and verified that it works on a test site. IMO it's RTBC. Similar patterns are currently being introduced for voting API, flag etc., so it would be consistent with that approach.

I would change drupal_get_path_alias('node/' . $trans_node->nid, $trans_node->language) into drupal_to_js(drupal_get_path_alias('node/'. $trans_node->nid, $trans_node->language)).

Additional shouldn't we test if the content translation module is active and only do this checks in this case?!? I'm not sure about this, but I thought it's the only way to create multilingual node's.

Yes, both changes suggested by hass are needed. Unless we test for translation module we'll trigger errors when it's not enabled.

I've made both those changes, tested locally and seems to be working well.

Thanks!

This is looking good and successfully addresses the issues hass raised.

Two things:

1. The existing calls are to url(), which gives the full path from the base path, while this is to drupal_get_path_alias(), which does not. Looks like the drupal_get_path_alias() needs to be wrapped in url().

2. Minor point, but it would be better to change $trans_node to $source_node or $source_translation because (a) we avoid abbreviations in variable names and (b) it's the source translation we're looking for.

Both changes are good, I wasn't keen on $trans_node but couldn't think of a good alternative, went for $source_node instead.

catch: thanks! This looks good to go.

hass (or other maintainers): Before applying this, I think there's one remaining question: should this be a configuration option? It seems likely that most sites will want the behaviour in this patch, i.e.: aggregate views of a piece of content in any translation, rather than tracking views of each translation separately.

But it's possible that some sites would want the existing behaviour. What do you think? Should this be a setting? If so, pls set this to needs work and we can revise the patch accordingly. If not, I think it's ready to go.

Setting to needs work until we have a version with the configuration option.

I also wondered about the comments in the patch, but haven't commented the logic without testing. As I understood the patch it only gives the original node back. I think this is very atypical behaviour.

Is it correct that:
1. node/34 is created in the default site language (English)
2. node/34 is translated to German as node/35
3. If a user read node/35 GA should track node/34?

This would be strange... I would understand that this makes sense for language neutral node's, but otherwise not very much. If I would run a site on example.com and example.de I would loose all info what language will be used mostly.

Yes, when a node is a translation, it gives the URL for the source node. Use cases include if you were running an e-commerce site (or any site dealing with CDs, films etc.) and wanted to track traffic per-item. It sounds like we definitely need to the configuration option. I should have a patch this evening, or at the latest tomorrow morning to add that in.

This is not very common... place the settings checkbox in the advanced settings, please. Additional, we should re-think if a fallback logic for nodes that are displayed in other languages shown with path detection, but not yet translated should have a special logic. This is duplicate content and a no-go logic from SEO side, but as I know it is possible and I'm not sure how we should deal with this stuff. This will become a more complex patch... and we should think about all possible language detection logics and if it makes sense to do something special.

From the point of view of SEO, duplicate content only affects which version of a page will be displayed (i.e. only one will, not both) - it doesn't negatively impact pagerank, and google has stated this publically I think. Additionally, that sounds more like a feature of the global_redirect module to me, or at least a follow-up patch if global_redirect doesn't cover it.

For now at least, I've added a setting to the 'advanced settings' fieldset, so attach that for review.

There seems to be a bug. This feature implements an uncommon behaviour people won't expect and therefore must default to 0 (disabled). variable_get('googleanalytics_translation_group', 1) must be 0, isn't it? I will do some testing tomorrow.

Aside - uninstall of the new variable is missing.

we should re-think if a fallback logic for nodes that are displayed in other languages shown with path detection, but not yet translated should have a special logic

@hass, thanks for taking the time to review. Could you explain a bit more what you have in mind here? Do you mean content where the node.translate value is set to 1 (needs updating)? Perhaps the issue is one that could be considered in a follow-up patch?

@catch: this looks good. Small issues:

1. The default should be the same for both variable_get() calls. In line with what hass has suggested, it should be 0 in both cases.

2. The term used in translation module is 'translation set' rather than 'translation group', so we should use that here, calling the variable 'googleanalytics_translation_set'.

@nedjo I think hass means when path negotiation with fallback is used on a site, and a given node might have /somepath and fr/somepath - both as valid paths - since there wouldn't be a (default) english translation yet.

Apologies for the variable calls, that was shoddy. Added in to hook_uninstall and fixed all references from 'translation group' to 'translation set'.

Nice, this looks good to go.

I think we're best staying focused on the issue of translation sets. While somewhat similar, language and path fallbacks are a distinct issue and can be addressed in a separate followup patch if needed. Does that work for you, hass?

Setting back to needs review pending confirmation from hass or another maintainer.

The other i18n stuff could become a follow up.

I'm unsure about some other features regarding i18n logics. I thought about domain based configuration options for GA. See my comment in http://drupal.org/node/132650#comment-1037509. I've written this in a context where I misunderstood ArjanLikesDrupal, but it made me thinking what could also be missing... and I'm not sure today how core would solve different settings per domain from UI side. Maybe you can point me to some docs or examples or simply acknowledge that i18n.module would solve this and we don't need to implement something in GA.

Patch does not apply cleanly.

Here's a re-roll. This one ought to apply OK.

Only for me - is there a developers rule I'm not aware about - that we need to break comments after 73 or 80 letters?

+    // If this node is a translation of another node, pass the original
+    // node instead.

crosspost

Comments should usually wrap after 80 characters, yes. I can't find the actual bit in the coding standards which say this right now, but it's discussed here: http://drupal.org/node/310904#comment-1037654

I think this is to performance intensive and also not required:
url(drupal_get_path_alias('node/' . $source_node->nid, $source_node->language))

This may do the trick...
url('node/'. $source_node->nid, array('language' => $source_node->language))

Only code wise... untested.

That's completely right, url() handles path aliases internally anyway.

edit: missed one occurrence of groups -> sets.

I've tested the patch and used a node/11 (German) with alias "de/impressum". Then switched over to the English version node/64 with alias "en/imprint" and checked the URL that is added to pageTracker._trackPageview("/drupal6/node/11"). This is not correct.

See #319993: url() with language does not return url_alias in specified language for the url() function issue that seems to be a bug for me.

I also tried url(drupal_get_path_alias('node/' . $source_node->nid, $source_node->language)), but this returns "en/impressum" and the Path should be "de/impressum" on a English node. Therefore this mixes the current node language prefix with the German alias of the source node. I've tried with "Path prefix with language fallback" setting.

Hmm, interesting bug. On my localhost, I can't get the path alias for a translated node to work at all - will try with a clean install and see if I can reproduce.

Additional fun could occur with domain based detection...

Found it.

#204106: Test for translated path aliases
This looks like a pretty serious core bug. Hopefully see you over there.

Following discussion with damz on that other issue and irc, here it is with the language as a proper language object instead of a string. That seems to be the only issue here. We could disable the option for 'domain based negotation' - but equally, people can not enable it for that.

This was a hard task for such a small patch :-). THX all of you.

Only a short comment about the domain detection. If you track all sites in one account it would look ok. It would only become "strange" if you have different account numbers per domain. Let's wait for someone really needs a crazy tracking :-)

Thanks Hass!