Support from Acquia helps fund testing for Drupal Acquia logo

Comments

andypost’s picture

Issue tags: +Novice

Tagging

dstol’s picture

Assigned: Unassigned » dstol
RoboPhred’s picture

Status: Active » Needs review
FileSize
2.33 KB

Assuming abandoned.
Here is a passthrough of the coder module. Coder found a lot more things to pick on aside from this one function inside locale.inc, but this patch was stripped down to only the function in question.

Status: Needs review » Needs work

The last submitted patch, 880278-cleanup_locale_inc.patch, failed testing.

RoboPhred’s picture

Status: Needs work » Needs review
FileSize
2.35 KB

Patch not from drupal root (Thanks Heline).

Status: Needs review » Needs work

The last submitted patch, 880278-cleanup_locale_inc.patch, failed testing.

RoboPhred’s picture

Abandoning coder module approach. Looked through the function and the coding standards document, and gave it a manual pass.
There wasn't much that I could find, just a few extra spaces.

RoboPhred’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: -Novice

The last submitted patch, 880278-cleanup_locale_inc-2.patch, failed testing.

RoboPhred’s picture

Status: Needs work » Needs review
Issue tags: +Novice

#7: 880278-cleanup_locale_inc-2.patch queued for re-testing.

andypost’s picture

Great! But comments should be on new line - http://drupal.org/node/1354#inline

RoboPhred’s picture

Things done

Moved comments to their own line.
More comments added and others clarified: needs review.
More linefeeds for clarity; I couldn't find any information on coding standards for this.
Converted " to ' where appropriate.

Possible bugs (no action taken)

msgctxt can be located before msgid. From what I am guessing with the syntax, this should only be definable after msgid. The end result is the same; msgctxt defined before msgid applies to the new id, not the previous one.

Possible improvements (no action taken)

I am unable to find documentation on what tokens are supported. Should that be under this function? Is there a separate location?

Markers for $context (MSGID, COMMENT, MSGID_PLURAL) are very similar, but do not match the keys used (msgid, #, msgid_plural). Should they be unified (allowing for $current[$context] = ...)?
$context might be clearer when renamed to $token to avoid confusion with the msgctxt (context) token.

Tokens check the current context (previous token) to ensure they can be used where they are located. However, this may be unclear in conditions where multiple tokens are working together, and adds complexity in determining if an incompatible token was used (the result of which is the bug listed above).
I recommend that instead of checking the context, the tokens check for the presence or absence of required or incompatible tokens using isset($current['token']). This would simplify the code and remove the possibility for errors by clearly explaining/enforcing the requirements and incompatibilities.
Example:
The code for 'msgstr[]', currently

    elseif (!strncmp('msgstr[', $line, 7)) {
      // A message string for a specific plurality.
      
      if (($context != 'MSGID') && ($context != 'MSGCTXT') && ($context != 'MSGID_PLURAL') && ($context != 'MSGSTR_ARR')) {
        // Message strings must come after msgid, msgxtxt, msgid_plural, or other msgstr[] entries.
        _locale_import_message('The translation file %filename contains an error: "msgstr[]" is unexpected on line %line.', $file, $lineno);
        return FALSE;
      }

would become

    elseif (!strncmp('msgstr[', $line, 7)) {
      // A message string for a specific plurality.
      
      // msgstr[] can only be used with a valid id, and is incompatible with the string usage of msgstr.
      if (isset($context['msgid']) && (!isset($context['msgstr]) || is_array($context['msgstr']) == TRUE)) {
        _locale_import_message('The translation file %filename contains an error: "msgstr[]" is unexpected on line %line.', $file, $lineno);
        return FALSE;
      }

Again, this is not part of the current patch. I am waiting for feedback on it.

Status: Needs review » Needs work

The last submitted patch, 880278-cleanup_locale_inc-2.patch, failed testing.

andypost’s picture

Interesting thoughts... maybe file another issue about?

4 tests are failed you touch only comments so what's wrong?

+++ includes/locale.inc	16 Feb 2011 00:28:00 -0000
@@ -586,159 +586,250 @@
-      $line = str_replace("\xEF\xBB\xBF", '', $line);
+      $line = str_replace('\xEF\xBB\xBF', '', $line);

Suppose this conversion caused broken tests

+++ includes/locale.inc	16 Feb 2011 00:28:00 -0000
@@ -586,159 +586,250 @@
+  ¶
+  /*
+   * The parser context.  Can be:

Patch contains a lot of trailing white-spaces.
There's 2 spaces before 'Can'

Powered by Dreditor.

RoboPhred’s picture

I wasn't aware that php did not parse escape sequences when single quotes were used, corrected that line.
Fixed double space, checked the rest of the patch for trailing white-spaces and removed them.

RoboPhred’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 880278-cleanup_locale_inc-3.patch, failed testing.

RoboPhred’s picture

Assigned: dstol » RoboPhred
Status: Needs work » Needs review
FileSize
12.04 KB

Missed another escape character quotation at msgid_plural. Thanks, test bot.
Unfortunately, I have yet to get tests to function on my local testing server, so I cannot test these locally yet.

andypost’s picture

Status: Needs review » Reviewed & tested by the community

Looks good for now!

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Looks good. Committed to CVS HEAD.

Status: Fixed » Closed (fixed)
Issue tags: -Novice

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