Comments

yoshz’s picture

StatusFileSize
new647 bytes

See attachment for my workaround.

GDrupal’s picture

A perfect work around indeed...but perhaps the best way to approach this is to add support for webform_tt so we can add some elements to the options array. So we don't sacrifice consistence. What do you think. It will imply to patch webform too, just a small patch, but that could be a little slow.

GDrupal’s picture

Status: Active » Needs review

Should be fixed by this commit . Please review and mark as fixed if it works.

GDrupal’s picture

Status: Needs review » Fixed

Working ok right now.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

fietserwin’s picture

Issue summary: View changes
Status: Closed (fixed) » Active

This (still) doesn't work in the 4.2 version, nor the current dev version (17-12-2015). I will post a patch shortly.

fietserwin’s picture

Status: Active » Needs review
StatusFileSize
new3.21 KB

Patch attached.

I tested this patch and it works fine for me.
- via node/{nid}/webform/configure
- via admin/config/regional/translate/translate
- and it works for the visitor.

fietserwin’s picture

StatusFileSize
new3.06 KB

Patch contains wrong folders, (starting at my site root instead of this project). Again

The last submitted patch, 7: confirmation_message-2040511-7.patch, failed testing.

The last submitted patch, 7: confirmation_message-2040511-7.patch, failed testing.

The last submitted patch, 7: confirmation_message-2040511-7.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 8: confirmation_message-2040511-7.patch, failed testing.

The last submitted patch, 8: confirmation_message-2040511-7.patch, failed testing.

The last submitted patch, 8: confirmation_message-2040511-7.patch, failed testing.

fietserwin’s picture

Status: Needs work » Needs review
StatusFileSize
new3.05 KB

I didn't test creating a new webform and setting the translation mode ...

(kind of) interdiff:

diff --git a/includes/webform_localization.i18n.inc b/includes/webform_localization.i18n.inc
@@ -17,7 +17,7 @@ index 1d44196..dcbfa83 100644
    if (!empty($properties['confirmation']['value'])) {
      $name = webform_localization_i18n_string_name($properties['nid'], 'confirmation');
-    i18n_string($name, $properties['confirmation']['value'], $options + array('format' => $properties['confirmation_format']['value']));
+    i18n_string($name, $properties['confirmation']['value'], $options + array('format' => $properties['confirmation']['format']));
    }
    if (!empty($properties['submit_text'])) {
      $name = webform_localization_i18n_string_name($properties['nid'], 'submit_text');
Philou88’s picture

With patch 9, with HTML choice, impossible to translate the message, I have a warning message.
I came back in Plain Text, and the translation impose

in the message : "

Thank you for your inquiry. We will contact you shortly.

"

fietserwin’s picture

Thanks for testing the patch.
- What is the warning message?
- Did you configure your site to allow to translate filtered and full html (admin/config/regional/i18n/strings)? If not:
* you will get a warning on trying to update the webform in a non default language ({langcode}/node/{nid}/webform/configure)
* you won't see the string at all in the translation interface (/admin/config/regional/translate/translate).

joseph.olstad’s picture

We likely need to add a sub module to automatically allow translate filtered and full html, then enable it automatically when string translation single webform option is selected.

This should help the related issues I was banging my head on

Thanks for the good idea, need to use features to create this sub module with required configuration .

This way we should be able to get this working out of the box

joseph.olstad’s picture

**EDIT** see previous comment

joseph.olstad’s picture

**EDIT** see above comment

fietserwin’s picture

Unless, I understand your remark incorrectly, I don't think we should "automatically allow translate filtered and full html". That is governed by an access right and thus should be obeyed.

joseph.olstad’s picture

Access rights for translating strings is what will control who can translate your webform. What the problem is now is that we don't have ANY reasonable filter settings out of the box. i18n_string_xss_admin is the most permissive filter that is out of the box for the i18n_string function however it's too restrictive and removes important tags. Our use case of i18n_string is unique in that our whole webform is translated this way when selecting the string translation method. We need more robust filter options but i18n_string does not provide this out of the box we have to therefore create our own filters.

Currently a webform is created , the filter options are only respected in one language, as soon as i18n_string kicks in its filtered again using the filter as defined in our i18n_string function calls , or else converted into plain text.

The way to fix this is to add filters, capture appropriate filters in a feature, put the feature in as a sub module, then when "single webform" options are selected in the webform_localization config options, to enable the sub module with the string filter settings, we'll have to also replace the i18n_string_xss_admin and i18n_string_xss filters with the sub module filters (could be an option in the gui for which type of filter to use) , then at that point rather than losing tags or having your html converted to plain text the translation will can have the same markup as the original webform as per the string translation.

At least this is what I'm thinking in theory. Got any other suggestions?

fietserwin’s picture

  • The text for the confirmation message in the original language has a format attached to it. Typically full_html or filtered_html, but basically any format that is available.
  • The orignal text is not further filtered or escaped using any of the functions Drupal provides (like check_plain, or filter_xss_admin). So it is the format here and the right to use that format that should provide the necessary security and access control.

IMO, this should thus be the same for the translated text:

  • Access to translate text under the given format (full_html or filtered_html) (compared to access to edit/create text with the given format): this is what i18n_string does for us.
  • Filtering the translated text in the same way as the original: this is what my patch does: pass the chosen format to i18n_string (and i18n_string uses the format option as expected).

So, I don't see any need to write our own filter functions or am I still missing something?

joseph.olstad’s picture

Status: Needs review » Closed (duplicate)
radubutco’s picture

StatusFileSize
new5.3 KB

I'm not sure this problem is solved on last dev version.

Rerolled the patch from #15 to work with last dev version.

Additionally I've changed the format option sent to i18n_string, from $node->webform['confirmation_format'] to I18N_STRING_FILTER_XSS, because the browser got stacked on full_html format option.

Also, I've changed all the misleading text 'poperties' into 'properties'.

radubutco’s picture

Status: Closed (duplicate) » Needs review

The last submitted patch, 1: webform_localization-2040511.patch, failed testing.

The last submitted patch, 1: webform_localization-2040511.patch, failed testing.

The last submitted patch, 1: webform_localization-2040511.patch, failed testing.

The last submitted patch, 1: webform_localization-2040511.patch, failed testing.

joseph.olstad’s picture

joseph.olstad’s picture

@radubutco thanks for the new patch. Since this is fresh in your memory, would it be too much trouble to ask for detailed steps to reproduce from a fresh D7 vanilla install? If its easier, include a node export of your webform, or a feature to capture some settings variables. This will help us and others save time involved in reviewing and testing the patch. It might lead to an additional simpletest test for this scenario.

I spent quite a bit of time on this and a similar issue, want to make sure I have the test scenario right.

joseph.olstad’s picture

Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

fgjohnson@lojoh.ca’s picture

Just downloaded the latest .dev version.
This is still an issue.
Strips out most of the html.

When I applied confirmation_message-2040511-25.patch it assumed Reversed (or previously applied) patch detected!

I choose Assume -R? [n] y and all components applied and the issue seems to be resolved.

Perhaps I need a refresher on patches.. :-)
...why did I need to elect to assume Y?

joseph.olstad’s picture

ok, so you're saying rolling this back fixed your problem?

It appears from what you described Assume -R
and you selected 'y'
then you rolled back patch #25

Unfortunately I don't have much time for this project lately, but I can rollback this commit and make a new release .

Please confirm that this solved the problem for you?

Seems that this might be a combination of configuration/code.
but if it works best without #24 then great.

joseph.olstad’s picture

Status: Closed (fixed) » Needs review

review rolling back #25

joseph.olstad’s picture

I rolled back patch 25 on the dev branch.

Going to push a new release in rolled back state.

Ideally NO html should be stripped at all. User input yes but not structure/config of Webform fields .

User input validation I Believe is handled entirely by webform not webform_localization. If I recall this is i18n_string forcing us to do this.

As a work around I suggest we roll back 25 as I have done and foradd documentation for theming webform using tpl instead of field label html and webform config.

Unless someone can finally fix this for good.

I Might think that we could permanently fix this by patching i18n_string adding a new more permissive filter.

This issue has dragged on way too long. Getting annoyed

joseph.olstad’s picture

Then again, the confirmation message is probably not themeable in a way that I might try to in a workaround as I suggested.

Previously immediately before patch 25 was committed and now presently that it is removed the confirmation message gets some basic html , whatever xml_admin filter will allow

fgjohnson@lojoh.ca’s picture

Joseph,

Thanks for revisiting.
I'll try out 4.9

whatever xml_admin filter will allow

seems to be the proper thing to me.

-------------
Sadly,
I have some custom bits that might be causing some additional pain for me!


(On a side note...
Do you know if Wetkit will be updating their version of webform_localization from 4.6? )

idflood’s picture

I believe there was some confusion in #36 / #37. I tried the latest dev version and the

tag was still escaped. I then applied the patch in #25 and the issue is resolved for me. I applied the patch with the following commands and it all went perfectly:

git clone --branch 7.x-4.x https://git.drupal.org/project/webform_localization.git
cd webform_localization
wget https://www.drupal.org/files/issues/confirmation_message-2040511-25.patch
git apply confirmation_message-2040511-25.patch
joseph.olstad’s picture

@idflood I suspect there may have been some custom filters in either your environment or @fgjohnsons` environment ,

It would be helpful to have a test scenario with detailed configuration /setup instructions from a fresh d7 installation, this would really be helpful to pinpoint what is going on here , consistency/inconsistency.

thanks for any assistance

fgjohnson@lojoh.ca’s picture

Joseph,
Sorry... long.

Tested in a default Wetkit from simplytest.me this am.

My issue may be related to Wetkit 4.12
Maybe I should open an issue @ the Wetkit issue queue?

webform - 7.x-4.14
wetkit webforms - 4.12-12+1dev
webform localization - 7.x-4.9
localization client - 7.x-1.3

WYSIWYG Text Filters / Order

  • Convert Media tags to markup
  • Make images responsive with the picture module
  • UUID Link filter
  • Correct faulty and chopped off HTML
  • Convert URLs into links

---------------------------------------------------------

Enabled WetKit Webform and it's dependencies.

Created a Webform

Added a field to that webform

Navigated to Webform / Form settings / Confirmation message and added an English message.
Saved the Form.

Clicked Translate / Add to add a french Translation of the Webform.
Clicked Webform / Paramètres du formulaire
Types in 3 lines of text.

When we save the Translation the WYSIWYG confirmation message appears like the following in the default WYSIWYG config.

This is the French Confirmation content as it appears in the WYSIWYG.
<p>Three lines</p> <p>One Token = <a title="Insert this token into your form">[current-date:long]</a></p>

Switching to Source we see that this has happened.
<p>&lt;p&gt;This is the French Confirmation content&lt;/p&gt; &lt;p&gt;Three lines&lt;/p&gt; &lt;p&gt;One Token = &lt;a title=&quot;Insert this token into your form&quot;&gt;[current-date:long]&lt;/a&gt;&lt;/p&gt;</p>

joseph.olstad’s picture

adding an old 6.x i18n issue as 'related' just as food for thought.

It'd be nice to get to the bottom of this issue once and for all.

Now that we know that the wetkit distro is affected, we just need a d7 vanilla test scenario to confirm that patch #25 actually works for that scenario.

Once it is determined if this is wetkit config or not, we can then open an issue in the wetkit queue for that.

TODO:
I'd like to know if patch #25 can work with d7 vanilla plus wetkit_localization dependencies and a minimalist configuration. We just need a test case scenario for that.

any assistance is appreciated

idflood’s picture

StatusFileSize
new3 KB

I will try to come up with a proper step-by-step to reproduce the issue on vanilla drupal but first here is a manual reroll of path #25 since it doesn't apply anymore (the comments have been fixed).

idflood’s picture

StatusFileSize
new1.05 KB
new3.18 KB

So here is a step by step way to reproduce it:

  • Download drupal 7.51 and install it with standard profile
  • Download i18n (7.x-1.14), webform (7.x-4.14) and webform_localization (7.x-4.9), ctools (7.x-1.11), views (7.x-3.14), variable (7.x-2.5), token (7.x-1.6) (drush dl i18n webform webform_localization ctools views variable)
  • Enable "Content translation", "Locale", "Internationalization", "Multilingual content", "String translation", "views", "ctools", "token" (drush en translation locale i18n i18n_node i18n_string views ctools webform webform_localization token)
  • Add a language in admin/config/regional/language
  • Enable multilingual support for the webform content type (enabled, with translation) at admin/structure/types/manage/webform
  • Add the language switcher (user interface text) block to the sidebar first region in admin/structure/block
  • Add url selection in admin/config/regional/language/configure
  • Create a test webform in english and add a simple textfield component for test
  • in the webform > form settings, enable "localization by string translation" (both "expose webform component…" and "Keep a single webform …" checkboxes) and save.
  • in confirmation message enter <p>This is a test<br>Will it keep my <em>tags?</em></p><p>another paragraph for test</p>
  • translate the webform at node/1/translate
  • refresh "Webform Localization" strings at admin/config/regional/translate/i18n_string
  • translate the confirmation message and keep the html tags at admin/config/regional/translate/translate (note that below there is no "format" notes)

Now if you submit the translated form the confirmation message should display the message exactly how we entered them (tags escaped, html code visible). In original language the message is displayed correctly.

So now we can apply the rerolled patch:

git clone --branch 7.x-4.x https://git.drupal.org/project/webform_localization.git
cd webform_localization
wget https://www.drupal.org/files/issues/confirmation_message-2040511-47.patch
git apply confirmation_message-2040511-47.patch

Once this is done:

  • go to update.php (this will clear cache).
  • update the "webform localization" string again at admin/config/regional/translate/i18n_string
  • edit again the translation of the confirmation message at admin/config/regional/translate/translate . Note that this time below the textare there is a note about the format (Standard XSS filter, which doesn't support <p> tag for instance). Hit "save translation" withtout changing anything.

Note that this time the <em> tag is kept but the <p> get losts (the message is wrapped by what i believe is a default <p>)

So I think there is a small missing piece in patch #25. Here is a new one which add a little twist in webform_localization_translate_strings. In that function #25 force the confirmation message to be in I18N_STRING_FILTER_XSS format, but interestingly we can get the real format confirmation format from $node->webform['confirmation_format'].

So now let's try to clone again the 7.x-4.x branch and apply the new patch. Once this is done:

  • Go to update.php, just in case
  • Go to admin/config/regional/i18n/strings and allow "Filtered HTML" format to be translatable.
  • Edit again the confirmation message by goin to node/1/webform/configure and hit save (note, don't do it on the translated node, this is another issue)
  • Refresh again the "webform localization" strings at admin/config/regional/translate/i18n_string
  • Edit the translation at admin/config/regional/translate/translate
  • Here we lost the previous translation (not sure about this, maybe was because I was hacking the module at the same time). But now at the bottom it shows that we are using the "Filtered HTML" text format : ) . So copy the original text and add something to see that we show the translated text afterwards.
  • Go to admin/config/content/formats/filtered_html and add <p> in the list of allowed tags
  • Now the tags are correctly kept
joseph.olstad’s picture

wow @idflood, great job! How do you suggest we go from here? So assuming we apply your patch, is there some sort of way we can warn the user if they have not allowed "Filtered HTML" format to be translateable? So that we can kind of help people along the way get this working correctly?

joseph.olstad’s picture

@fgjohnson , any thoughts? with instructions by @idflood, does this help you get it (his patch) working in your custom environment/distro? and more importantly, pinpoint the divergence

idflood’s picture

@joseph.olstad I think we should double check that whether or not these translation get lost. If this is really the case and the new patch still looks like the way to go then:

  1. If possible try to find a way to fix these string translation in a update hook. The translation key doesn't change so the fix should be possible (maybe it's just a column to update in the table with the proper format)
  2. If it's not possible to fix that in a update hook then why not bump the project version to 5.x with a big warning.

Eventually the solution n°2 would be really sad : / I will try to replicate the issue and try to debug what happens in the database.

idflood’s picture

I found why the translation got deleted. It happen because the confirmation message format is not in the "translatable text formats". If I refresh the webform localization string while the "filtered html" format is not checked then this message is displayed:

The string webform:1:confirmation for textgroup webform is not allowed for translation because of its text format.

Once it happens the entry about the confirmation message in the table i18n_string get deleted. If I first enable "Filtered HTML" translation and only after update the webform localization strings then the message translation is not deleted.

So one potential fix to this issue would be to first check that the format is allowed before refreshing the confirmation translation string. If it is not then we could display a warning message like "The string […] could not be refreshed because the text format XYT is not allowed for translation".

idflood’s picture

Well, I tried to simply add some condition in webform_localization_translate_strings but it's not really successful yet (too much warning repeated and I'm not sure it's the correct way). For reference I replaced:

if (isset($node->webform['confirmation_format'])) {
  $confirmationOptions['format'] = $node->webform['confirmation_format'];
}

With something like that:

  if (isset($node->webform['confirmation_format'])) {
    if (i18n_string_allowed_format($node->webform['confirmation_format'])) {
      $confirmationOptions['format'] = $node->webform['confirmation_format'];
    }
    else {
      drupal_set_message(t('The string @name could not be refreshed with the text format @format because it is not allowed for translation.', array(
        '@name' => $name,
        '@format' => $node->webform['confirmation_format']
      )), 'warning');
    }
  }
idflood’s picture

StatusFileSize
new2.58 KB
new4.1 KB

Here is a new patch which tries to prevent translation text loss. So what it does is that it keeps by default the I18N_STRING_FILTER_XSS format, and only if the confirmation format is supported use it for the translation. If it is not then a message is displayed informing about the impossibility to use this format.

I will try to test it on some websites to see if everything goes well but it would be awesome if we could get some other confirmation.

joseph.olstad’s picture

Hi @idflood , great work again, I'm hoping that @fgjohnson can try this patch on his environment as well.

I'll run some of my own tests soon, we're working on some new webforms as we speak so we'll be needing this.

joseph.olstad’s picture

Ok, we did some more testing, according to our tests, version 4.8 with the original patch was actually working fairly well but did strip some html but overall was better than without patch

Version 4.9 (no patches) the HTML gets rendered into HTML entities, so nope, not good.

Now with patch #54 we're back in business and our translation appears to reflect what was in the source language. Great work @idflood

gdaw’s picture

Nice patch folks, 4.8 stripped some HTML and 4.9 translation was full-on HTML displayed, but this latest patch keeps even the most complicated HTML intact.

RTBC + 1 , and thumbs way up!

  • joseph.olstad committed 6b6c110 on 7.x-4.x authored by idflood
    Issue #2040511 by idflood, fietserwin, radubutco, yoshz: Confirmation...
joseph.olstad’s picture

Status: Needs review » Fixed
fgjohnson@lojoh.ca’s picture

idflood Your head must be spinning... mine is just reading! :-)

I will try to confirm all this in the next couple of days...

fgjohnson@lojoh.ca’s picture

Saw that this was committed to 7.x-4.10

Tested in Wetkit 4.12.
Seems to be fixed there too.

joseph.olstad’s picture

@fgjohnson, thanks for the followup, good to hear this issue is finally resolved.

joseph.olstad’s picture

might be nice to get some extra test coverage on this though ( an extra assert in a simpletest)

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.