Merge pull request #446 from flodolo/checkvar

 Improve check variables controls, switch to MVC

app/classes/Transvision/AnalyseStrings.php

@@ -44,54 +44,70 @@ public static function differences($tmx_source, $tmx_target, $repo, $ignored_str
 
         if (Strings::startsWith($repo, 'gaia')) {
             $patterns = [
-                'l10njs'     => '/\{\{([\s]*[a-z0-9]+[\s]*)\}\}/i', // {{foobar2}}
+                'l10njs'     => '/\{\{([\s]*[a-z0-9_]+[\s]*)\}\}/i', // {{foobar2}}
             ];
         } else {
             $patterns = [
-                'dtd'        => '/&([a-z0-9\.]+);/i',                // &foobar;
-                'printf'     => '/%([0-9]+\$){0,1}([0-9].){0,1}S/i', // %1$S or %S. %1$0.S and %0.S are valid too
-                'properties' => '/(?<!%[0-9])\$[a-z0-9\.]+\b/i',     // $BrandShortName, but not "My%1$SFeeds-%2$S.opml"
+                'dtd'        => '/&([a-z0-9\.]+);/i',                        // &foobar;
+                'printf'     => '/(%(?:[0-9]+\$){0,1}(?:[0-9].){0,1}(S))/i', // %1$S or %S. %1$0.S and %0.S are valid too
+                'properties' => '/(?<!%[0-9])\$[a-z0-9\.]+\b/i',             // $BrandShortName, but not "My%1$SFeeds-%2$S.opml"
+                'l10njs'     => '/\{\{([\s]*[a-z0-9_]+[\s]*)\}\}/i',         // {{foobar2}} Used in Loop and PDFViewer
             ];
         }
 
         foreach ($patterns as $pattern_name => $pattern) {
-            foreach ($tmx_source as $key => $value) {
+            foreach ($tmx_source as $key => $source) {
                 if (isset($tmx_target[$key])
                     && $tmx_target[$key] != ''
                     && ! in_array($key, $ignored_strings)) {
                     // Check variables only if the translation exists and
                     // the string is not in the list of strings to ignore
                     $translation = $tmx_target[$key];
                     $wrong_variable = false;
-                    preg_match_all($pattern, $value, $matches_source);
+
+                    preg_match_all($pattern, $source, $matches_source);
+                    preg_match_all($pattern, $translation, $matches_translation);
+
                     if (count($matches_source[0]) > 0) {
-                        foreach ($matches_source[0] as $val) {
-                            if ($pattern_name == 'printf') {
-                                // Variables are in format %S or %1$S, case is not relevant
-                                if (stripos($translation, $val) === false) {
-                                    $wrong_variable = true;
-                                }
-                            } else {
-                                if (strpos($translation, $val) === false) {
-                                    $wrong_variable = true;
+                        foreach ($matches_source[0] as $var_source) {
+                            if (! in_array($var_source, $matches_translation[0])) {
+                                $wrong_variable = true;
+                            }
+
+                            // For l10n.js {{n}} == {{ n }}
+                            if ($pattern_name == 'l10njs' && $wrong_variable) {
+                                // Trim whitespaces and sort variables alphabetically
+                                $trimmed_source_vars = array_map('trim', $matches_source[1]);
+                                sort($trimmed_source_vars);
+                                $trimmed_source_trans = array_map('trim', $matches_translation[1]);
+                                sort($trimmed_source_trans);
+
+                                if ($trimmed_source_vars === $trimmed_source_trans) {
+                                    $wrong_variable = false;
                                 }
                             }
                         }
                     }
 
                     if ($pattern_name == 'printf') {
-                        preg_match_all('/%([0-9]+\$){1}([0-9].){0,1}S/i', $translation, $matches_ordered);
-                        preg_match_all('/%(0.){0,1}S/i', $translation, $matches_unordered);
+                        /*
+                         * Check ordered vs unordered variables. The pattern
+                         * regular expression returns "S" or "s" as second group
+                         * for each variable, I can use it to check for case
+                         * differences ("s" vs "S") which are not allowed.
+                         */
+                        preg_match_all('/%(?:[0-9]+\$){1}(?:[0-9].){0,1}S/i', $translation, $matches_ordered_trans);
+                        preg_match_all('/%(?:0.){0,1}S/i', $translation, $matches_unordered_trans);
 
-                        if ((count($matches_ordered[0]) > 0) &&
-                            (count($matches_unordered[0]) > 0)) {
-                            // A string can't have both ordered and unordered variables
+                        if (count($matches_ordered_trans[0]) > 0 && count($matches_unordered_trans[0]) > 0) {
+                            // Strings can't mix ordered and unordered variables
                             $wrong_variable = true;
-                        } elseif (count($matches_ordered[0]) + count($matches_unordered[0]) == count($matches_source[0])) {
-                            /* I have the same number of variables in source and translation,
-                             * I consider the string valid.
-                             */
-                            $wrong_variable = false;
+                        } else {
+                            if (count($matches_translation[0]) == count($matches_source[0]) &&
+                                $matches_translation[2] === $matches_source[2]) {
+                                // Same number of variables and case, I assume the string is OK
+                                $wrong_variable = false;
+                            }
                         }
                     }
 

app/controllers/check_variables.php

@@ -0,0 +1,8 @@
+<?php
+namespace Transvision;
+
+// Get requested repo and locale
+require_once INC . 'l10n-init.php';
+
+include MODELS . 'check_variables.php';
+include VIEWS . 'check_variables.php';

app/inc/dispatcher.php

@@ -81,10 +81,9 @@
         $page_descr = 'Display a list of strings identical to English';
         break;
     case 'variables':
-        $view = 'checkvariables';
-        $experimental = true;
+        $controller = 'check_variables';
         $page_title = 'Variables Overview';
-        $page_descr = 'Show potential errors in your strings for the use of variables.';
+        $page_descr = 'Show potential errors related to missing or mispelled variables in your strings.';
         break;
     case 'productization':
         $view = 'productization';

app/models/check_variables.php

@@ -0,0 +1,48 @@
+<?php
+namespace Transvision;
+
+$source = Utils::getRepoStrings(Project::getReferenceLocale($repo), $repo);
+$target = Utils::getRepoStrings($locale, $repo);
+
+// Set up channel selector, ignore mozilla.org
+$channels = Project::getSupportedRepositories();
+unset($channels['mozilla_org']);
+$channel_selector = Utils::getHtmlSelectOptions($channels, $repo, true);
+
+// Build the target locale switcher
+$target_locales_list = Utils::getHtmlSelectOptions(
+    Project::getRepositoryLocales($repo),
+    $locale
+);
+
+$source = array_map(['Transvision\AnalyseStrings', 'cleanUpEntities'], $source);
+$target = array_map(['Transvision\AnalyseStrings', 'cleanUpEntities'], $target);
+
+// We need to ignore some strings because of false positives
+$ignored_strings = [
+    'mail/chrome/messenger/aboutRights.dtd:rights.webservices-term4',
+    'suite/chrome/branding/aboutRights.dtd:rights.webservices-term4',
+    'toolkit/chrome/global/aboutRights.dtd:rights.webservices-term5',
+];
+
+$var_errors = AnalyseStrings::differences($source, $target, $repo, $ignored_strings);
+$error_count = count($var_errors);
+
+// Add component filter
+if (in_array($repo, $desktop_repos)) {
+    // Build logic to filter components
+    $javascript_include = ['component_filter.js'];
+    $components = Project::getComponents(array_flip($var_errors));
+    $filter_block = '';
+    foreach ($components as $value) {
+        $filter_block .= " <a href='#{$value}' id='{$value}' class='filter'>{$value}</a>";
+    }
+}
+
+// RTL support
+$direction1 = RTLSupport::getDirection($source_locale);
+$direction2 = RTLSupport::getDirection($locale);
+
+$bugzilla_base_link = 'https://bugzilla.mozilla.org/enter_bug.cgi?format=__default__&component=' .
+                      Bugzilla::getURLencodedBugzillaLocale($locale, 'products') .
+                      '&product=Mozilla%20Localizations&status_whiteboard=%5Btransvision-feedback%5D';

app/views/check_variables.php

@@ -0,0 +1,76 @@
+<?php
+namespace Transvision;
+
+// Include the common simple search form
+include __DIR__ . '/simplesearchform.php';
+
+if ($error_count > 0) {
+    $content = "<h2>{$error_count} " .
+               ($error_count == 1 ? 'result' : 'results') .
+               " found</h2>\n";
+
+    if (isset($filter_block)) {
+        $content .= "<div id='filters'>" .
+                    "  <h4>Filter by folder:</h4>\n" .
+                    "  <a href='#showall' id='showall' class='filter'>Show all results</a>\n" .
+                    $filter_block .
+                    "</div>\n";
+    }
+
+    $content .= "<table class='collapsable'>\n" .
+                "  <tr>\n" .
+                "    <th>Entity</th>\n" .
+                "    <th>{$source_locale}</th>\n" .
+                "    <th>{$locale}</th>\n" .
+                "  </tr>\n";
+
+    foreach ($var_errors as $string_id) {
+        // Link to entity
+        $string_id_link = "?sourcelocale={$source_locale}" .
+                          "&locale={$locale}" .
+                          "&repo={$repo}" .
+                          "&search_type=entities&recherche={$string_id}";
+        $bug_summary = rawurlencode("Translation update proposed for {$string_id}");
+        $bug_message = rawurlencode(
+            html_entity_decode(
+                "The string:\n{$source[$string_id]}\n\n" .
+                "Is translated as:\n{$target[$string_id]}\n\n" .
+                "And should be:\n\n\n\n" .
+                "Feedback via Transvision:\n" .
+                "http://transvision.mozfr.org/{$string_id_link}"
+            )
+        );
+        $bugzilla_link = "{$bugzilla_base_link}&short_desc={$bug_summary}&comment={$bug_message}";
+
+        $path_locale1 = VersionControl::hgPath($source_locale, $repo, $string_id);
+        $path_locale2 = VersionControl::hgPath($locale, $repo, $string_id);
+
+        $component = explode('/', $string_id)[0];
+        $content .= "<tr class='{$component}'>
+                       <td>
+                          <span class='celltitle'>Entity</span>
+                          <a class='link_to_entity' href=\"/{$string_id_link}\">" . ShowResults::formatEntity($string_id) . "</a>
+                       </td>
+                       <td dir='{$direction1}'>
+                          <span class='celltitle'>{$source_locale}</span>
+                          <div class='string'>" . Utils::secureText($source[$string_id]) . "</div>
+                          <div class='result_meta_link'>
+                            <a class='source_link' href='{$path_locale1}'><em>&lt;source&gt;</em></a>
+                          </div>
+                       </td>
+                        <td dir='{$direction2}'>
+                          <span class='celltitle'>$locale</span>
+                          <div class='string'>" . Utils::secureText($target[$string_id]) . "</div>
+                          <div class='result_meta_link'>
+                            <a class='source_link' href='{$path_locale2}'><em>&lt;source&gt;</em></a>
+                            <a class='bug_link' target='_blank' href='{$bugzilla_link}'>&lt;report a bug&gt;</a>
+                          </div>
+                       </td>
+                     </tr>\n";
+    }
+    $content .= "</table>\n";
+} else {
+    $content = "<h2>Congratulations, no errors found.</h2>";
+}
+
+print $content;