Support from Acquia helps fund testing for Drupal Acquia logo

Comments

joel_osc created an issue. See original summary.

joel_osc’s picture

Status: Active » Needs review
FileSize
721 bytes

Here is a small patch.

joel_osc’s picture

Slight modification to the patch...

hamrant’s picture

Status: Needs review » Reviewed & tested by the community

Looks good for me

nicrodgers’s picture

Status: Reviewed & tested by the community » Needs work

The patch applies ok, but doesn't seem to solve the problem.

Steps to replicate:
* Create a webform
* Enable 'expose strings for string translation'
* Add a pagebreak component
* View the form
* Go to Translate Interface: Refresh strings
* After refreshing, go to Translate Interface and filter by Webform Localization
* Look for the pagebreak component: it should be there, but it's not

nicrodgers’s picture

Status: Needs work » Needs review
FileSize
1.22 KB
1.29 KB

This is working for me locally...

joel_osc’s picture

I am using the first patch and it works without an issue. Your patch seems a little odd since it translates the 'value' of a page break which I am not really sure what that is. The component name/title is what is used in the progress bar which is normally the part that requires translation on all components. Let me do some testing, but as of now I am not sure the patch in #6 is the correct approach.

Cheers.

joel_osc’s picture

I just re-tested and I think it would be good to look closer at the string refresh code in webform_localization - even without this patch the string refresh should add the pagebreak #title string to the translations:

function webform_localization_i18n_string_refresh($group) {
  if ($group == 'webform') {
    global $language;
    $languages = language_list();
    $source_language = $languages[i18n_string_source_language()];
    // Refresh the strings using source language.
    $language = $source_language;
    module_load_include('inc', 'webform_localization', 'includes/webform_localization.i18n');

    // In case updating before UUID support.
    if (module_exists('uuid') && !variable_get('webform_localization_using_uuid', FALSE)) {
      webform_localization_uuid_update_strings(FALSE);
    }

    // Get components configured as translatable.
    $query = db_select('webform_component', 'wc');
    $query->fields('wc');
    $query->condition('wl.expose_strings', 0, '>');
    $query->innerJoin('webform_localization', 'wl', 'wc.nid = wl.nid');
    $components = $query->execute()->fetchAll();

    foreach ($components as $component) {
      $component = (array) $component;
      $component['extra'] = unserialize($component['extra']);

      webform_localization_component_update_translation_strings($component);

      $component['extra'] = serialize($component['extra']);
      drupal_write_record('webform_component', $component, array('nid', 'cid'));
    }

I think your patch works because you call i18n_string with update => TRUE which then creates the string which really should already be there. Perhaps you could remove your patch from the progress bar label code and re-test the string refresh and debug why it didn't pick up your page break?

nicrodgers’s picture

Hi Joel, thanks for taking a look at it and providing feedback.

It's interesting that the first patch works ok for you. With patch #2, and the steps to replicate in #5, can you confirm you see the page break component after refreshing? I've tried on an existing site, and a clean install, and both times it does not work. Or are you doing something different?

I did some debugging, which led to patch #6. At first, I simply modified the patch in #2 to change update => FALSE to update => TRUE, and after viewing the form in another language, the string became available for translation. After translating the string in the translate interface, the webform does display the correct translation. However, if you then use Refresh Strings, the translated string is deleted and will only re-appear for translation (losing your original translation) when the webform is next viewed in another language.

Looking at webform_localization_i18n_string_refresh, it gets all components of translatable forms, and then loops through each component, calling webform_localization_component_update_translation_strings. That function then calls to get the 'render' FAPI array (and not 'display') for the component. For pagebreaks, it calls _webform_render_pagebreak to get the render array. That function outputs a hidden form element, it's value attribute is set to the name of the pagebreak (and being a hidden element, doesn't have a title). It then looks at the render array for the #translatable key. That's why I added a #translatable property of 'value', and modified the original patch to use value instead of title.

.... Which got me wondering, there is code in webform_localization_component_update_translation_strings to get the 'display' render FAPI array, and then merge it with the 'render' array. However, it's commented out. Looking at the commit history of webform_localization, it was commented out in 5b64a8e44e0835eafc3065b3330754accfb3ad3f #2633228-18: Field labels displayed as placeholders. My guess is that Joel you are using a version of webform_localization prior to this change, which explains why the #2 works ok for you?

If that is the case, we need to know whether the commenting out was intentional or not. If it was accidental, we can reverse the change and use Joel's patch in #2.

joel_osc’s picture

Hey, excellent work! I think you are correct about the patch - let me do some testing and see what happened with that patch. Cheers!

joel_osc’s picture

Hey @nicrodgers, you are correct that line of the patch breaks page break translations. I am not sure why it is there. I will put a comment in the other issue and continue investigation - perhaps the person who wrote the patch can shed some light on why that line was commented out.

nicrodgers’s picture

Nice one, @joel_osc.

nicrodgers’s picture

I noticed this patch doesn't work correctly with translations that have apostrophes in.

I traced it down to the default sanitize = TRUE setting within i18n_string_format, which runs the string through check_plain(). However, the webform--progressbar.tpl.php also runs the page_labels through check_plain. So apostrophe's are rendered as ' instead of '.

So I've updated the patch in #6 to pass the setting of sanitize = FALSE, which fixes the issue. Not sure if that's the best approach though?

joel_osc’s picture

FYI - the patch in the related issue that broken my original patch has been fixed so we should likely go back to my patch in #3 with your sanitization fix. If you are okay with that I will re-roll the patch. Cheers.

nicrodgers’s picture

Status: Needs review » Needs work

Yup, happy with that. Thanks joel!

nicrodgers’s picture

Status: Needs work » Needs review
FileSize
874 bytes
794 bytes

Hey Joel, I'm working on a project today that needs this, so I've gone ahead and rerolled your patch from #3 with the sanitize fix from #13. Is this what you had in mind? Cheers, Nic

joel_osc’s picture

Perfect, thanks for all of you help on this!

Liam Morland’s picture

Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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