Redirect webform config tabs to the original webform when using "single webform across translation set"

@rv0: This is a interesting issue.. We have to think more about it. What about this... We replace the webform tab for a single menu entry that redirects to the source webform we can call it "Source Webform" or something like that.

@rv0: Sound Good! Let me know if I can assist you in anything, will be my pleasure!

Can you further explain the core issue here?

If you are using the string translation method and not the content translation method then you should not have a "translation set". There should be only 1 node so why are you trying to redirect from translated nodes?

It sounds like you are trying to mix the options available in a bad way as if you are using content translation your webform results and options will be different per language and will need the webform tab to see and use those features.

No that is mixing it up. There are two options available that you can choose:

1. Use a single node and use the i18n_strings module to translate the labels etc. to translate the form on display the same way a menu item is. You miss out on node title translation with this method but if you don't need it that is fine.

2. (This is the method you quoted above though it is badly worded.) You can use content translation to translate the node into translation set giving the added benefit of full control over the translated node (add/remove fields, change options, etc.) which also allows the title field of the node to then be translated if you need it (most do because that is the only way to do it right now) but when doing so the user will be shown the translated node in the set and so their submition will be linked to that translated node and not the original node. The wording of keeping a "single" webform comes from being able to use i18n sync functionality to sync the common fields between the nodes in the translation set. This syncing however does not move the results or prevent you from changing the form on a specific node of a translation set that you need to be different.

Unfortunately you are wrong but it is obvious it is because the description is badly worded.

When i say content translation, I am not talking about entity_translation module which allows you to translate fields (which the title field is not a "field" in the fieldable system so it is not translatable via entity_translation), I am talking about the core Content Translation module (translation.module) which allows you to create multiple nodes and keep them in a translation set (which at the current time is the best way to translate the title).

We, and most people, are using the same methods you are. However, this module does not use a technical "single webform" across the nodes in a translation set, it allows you "sync" the webform fields from one node in the set to the other nodes in the set which gives the appearance of a single webform because you only have to edit one to edit them all however they are still separate and so keep separate results etc. This gives great flexibility but comes at some cost the same as similar situations.

@rv0 :
I will act as a bridge here between @dragonwize and you.

@dragonwize:
The scenario when you need to translate nodes using content translation and i18n integration is expected. @rv0 is right about that, he is talking only about a usability issue with all the webform tab being unusable most of the time.

But and i'm talking to both right now.... I realize from your discussion that a new scenario emerges. What will happen perhaps if you have a translation set with 4 languages and 3 of them can work with i18n_integration as @rv0 is using it and the last node needs a completely different webform (cultural reasons i don't know), but you want to keep some properties synchronized between the source webform and the different one.

I'm thinking Ouch! I have not foresee that.... so very interesting stuff there. In a case like that, we need a more general option to let you choose wich node does not need the webform tab and which one does. Or perhaps showed it by default and add an option to "replace it /hide" it when the webform its empty.

I dont know for sure a lot to think.....

@rv0: I will review this indeed!
I'm in the middle of a new module (media plugin) but i will make sometime and let you know ASAP.
Thank you for all this great work!

Well if you understand me then I will go back to my original point.

You have to have the webform tab on translated nodes so it would be a horrible user issue to redirect or remove that tab because it is needed to edit, see results, etc. that weform so it has to be there for the user to use. Which makes this patch "closed (won't fix)".

I won't change the status now because I feel like there is still something in our conversations that is missing but based on everything you have said so far that is my conclusion.

Maybe I do not. What it appears to be from the title of this issue, the text in the issue, and the code in #15, it is trying to redirect the user to the source webform config from the translated config. The problem with that is that, that should never be done. That would be horrible to not be able to get the to appropriate webform because you are always redirected to the source form when the user needs to be able to access that version of the form because while some fields may be synced sometimes they are not the same and can be changed.

I have to agree with @rv0 on this... Here there is a resume of a long chat with dragonwize just to clarify this scenario:

You can also use the i18n_string integration with node translation by the single_webform feature.
What it does is: use the webform in the source node and displayed in all nodes in the translation set with the proper translation from i18n_string. if you have exactly the same components with the same options just in different language this is all you need.

This is the scenario where @rv0 needs to implement this feature. In this scenario we have a bunch of empty webfroms, since the one and only webform is configured only from the source node.

The only complication that I see on this, is the emergent scenario that I mention on http://drupal.org/node/1481594#comment-5736196

I hope that now we are all in the same page?

I'm in the process of reviewing http://drupal.org/node/1481594#comment-5736132 still not so sure about a couple of things so I will test it more deeply. In relation to my observations in http://drupal.org/node/1481594#comment-5736196 i have added a little changes so a new patch. I will continue analyzing the code to be sure we are no missing or breaking something.

@rv0: Sorry about this, but I was focus on a first commit for 2 other modules... I have to discuss some of this changes with the sponsor of the module for now is the only blocker. I will let you know ASAP. Are you in a hurry?

FWIW, I'd be inclined to implement webform_menu_to_arg() to just make the local task tabs go to the correct URLs in the first place. I can't think of a reason why webform itself would ever implement this, so it's highly unlikely to ever conflict.

edit: Logged a webform feature request #2097277: Consider alterable webform_menu_to_arg() implementation

I agree that having Webform related tabs (Webform and Results) on a translated node are kind of misleading in case of "Keep a single webform across a translation set.".

However, another problem is that, when I submit a translated node Webform then I get an Access denied message. Yes I know that the results are saved on a different (related) node but still... Confirmation message is would be very user friendly to have a Thank you message :)

I can tackle this by setting my form Redirection location to Custom URL or No redirect (reload current page) ... with maybe some template tricks.

mareks: Surely that is (a) unrelated to this issue, and (b) well-covered by #2096583: After submitting translated node webform I get 403 Access denied (which you had already logged) ?

I' ve rerolled the patch from #15 witch works at my setup to use with version 4.x.
Whats the future of this issue?

The future of this issue is (or should be) that #2097277: Consider alterable webform_menu_to_arg() implementation allows us to fix it properly. We won't need any redirects, because all the links will point to the correct URL in the first place.

I do plan to provide a patch for this, now that #2097277 has been committed.

@jweowu I know it's been 11 months since the last update on this issue but any chance of a follow up?

future of this issue is .... allows us to.... fix it properly

this was fixed in the parent issue

Hi Joseph. My apologies for not following up your previous message; I should have posted a patch for this before now. #2097277: Consider alterable webform_menu_to_arg() implementation is really a child of this issue, not the other way around.

Here is a re-roll (based on this upstream change to webform) of the custom patch I've been using since September 2013.

Hi Jweowu , thanks for the patch, I reviewed, it looks pretty good and passes tests however I'm wondering , do we need a check to see if the option for single webform?
ex:

if ($webform_localization_options['sync_components']) {
 ....
}

so maybe line 264 should be this:

    if ($nid = webform_localization_single_webform_nid($node) && $webform_localization_options['sync_components']) {

instead of:

    if ($nid = webform_localization_single_webform_nid($node)) {

?

what modes/options does /or should this functionality apply to?

Be careful with your parentheses vs operator precedence. That variant would need to be if (($nid = webform_localization_single_webform_nid($node)) && $webform_localization_options['sync_components']) {

I assume you meant $webform_localization_options['single_webform'] rather than $webform_localization_options['sync_components'], though?

I haven't looked at this module for a long time, but I guess the answer is "probably"? I suspect it saves an unnecessary database query when the single_webform option isn't being used.

The test elsewhere seems to be explicitly ($webform_localization_options['single_webform'] > 0) rather than a simple boolean value, so I imagine that should be maintained for consistency at least.

Note also that webform_localization_node_view() was not making any such test. That function was the original source of the code that I've moved into webform_localization_single_webform_nid(). If we're adding in the additional test, we should probably put it inside webform_localization_single_webform_nid() (returning FALSE if it's not set) and therefore gain the benefit in the webform_localization_node_view() case as well; but we need to be certain that it is also correct in that case.

A look at webform_localization_get_config() makes me dubious that the single_webform option is going to be present for all the non-tnid nodes in the translation set?

...yes, calling that function for a non-tnid node gives you data for the (unused) webform associated with that specific (non-tnid) node, rather than the 'single' webform data for the tnid for that translation set.

All of which makes me think the original code was correct to not be testing the option.

We could be checking the single_webform value of webform_localization_get_config($node->tnid), OTOH. I believe that's just another way of checking the exact same data, but you might still prefer to do it that way?

Also notable is that webform_localization_preprocess_webform_confirmation() performs an almost-identical query, but with one difference in the WHERE clause, which is that one of the queries requires that wl.single_webform <> 0, and the other does not.

However I believe they are intended to be doing the same thing, in which case they could both be abstracted into a call to webform_localization_single_webform_nid().

That difference between the two queries is mitigated by the opening block of webform_localization_preprocess_webform_confirmation(), which returns immediately when empty($vars['node']->tnid), which means that adding the query constraint that wl.single_webform <> 0 would be a no-op for that function.

n.b. This is the case because a separate constraint requires that wl.single_webform is equal to $node->tnid

Looking back at the patch again,

    if ($nid = webform_localization_single_webform_nid($node)) {

was probably meant to be

    if ($nid == webform_localization_single_webform_nid($node)) {

haven't yet had time to look closely at this but at a glance I'd say it should probably be using the == operator as we're not assigning anything we would be comparing the nid in this if statement

No, absolutely not.

if ($nid = webform_localization_single_webform_nid($node)) {...} is assigning a value to $nid, and then testing it in a boolean context.

(because the value returned by the assignment operator is the value which was assigned.)

It is equivalent to:

$nid = webform_localization_single_webform_nid($node);
if ($nid) {...}

ah ok, thanks.

Hi jweowu , referring to comment #39 yes I meant

$webform_localization_options['single_webform']

good observation.

Based on your comments I have come up with a new patch, I added in the check for 'single_webform' in the webform_localization_single_webform_nid function as you suggested.

Please review this patch and interdiff and let me know if you think this makes sense. (and if possible test it for me? :) )

Thanks a bunch for your patch and assistance.

need to re trigger the testbot

reverting back to patch 37.

We'll have to try testing patch 37 manually to see if it is compatible with the other webform_localization modes ('single_webform' should work according to simpletest , would be nice to add some tests to simpletest for the other types, probably better to make more simpletest tests to do this for us.)

For clarity, I actually said (in comment 40) that testing the value of single_webform for the current node was the wrong thing to do (but that we could potentially do that for the translation source node for the current node's translation set -- much like I've done in _webform_localization_mollom_form_alter() in my patch you committed for #2137195: Mollom support for the 'single webform' localization option, in fact).

I do think this code could be cleaned up some more, so I'll try to revisit this later on. I think #37 is most likely usable, though (I've been running a near-identical patch in a production system for the past two years).

how about we commit patch #37 as is, if there's any issues with it we'll deal with it as it comes. As you say, you've been using a close version of it in production for 2 years.

committed, thanks jweowu