Posted by andypost on August 11, 2010 at 5:08pm
5 followers
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | locale.module |
| Category: | task |
| Priority: | normal |
| Assigned: | RoboPhred |
| Status: | closed (fixed) |
| Issue tags: | Novice |
Issue Summary
This is a follow-up from #860154-25: The locale module does not import the plural forms from the compact files
_locale_import_read_po() does not conform core's code-stiling
Comments
#1
Tagging
#2
Once #860154: The locale module does not import the plural forms from the compact files is committed I'll clean up the function.
#3
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.
#4
The last submitted patch, 880278-cleanup_locale_inc.patch, failed testing.
#5
Patch not from drupal root (Thanks Heline).
#6
The last submitted patch, 880278-cleanup_locale_inc.patch, failed testing.
#7
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.
#8
#9
The last submitted patch, 880278-cleanup_locale_inc-2.patch, failed testing.
#10
#7: 880278-cleanup_locale_inc-2.patch queued for re-testing.
#11
Great! But comments should be on new line - http://drupal.org/node/1354#inline
#12
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.
#13
The last submitted patch, 880278-cleanup_locale_inc-2.patch, failed testing.
#14
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.
#15
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.
#16
#17
The last submitted patch, 880278-cleanup_locale_inc-3.patch, failed testing.
#18
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.
#19
Looks good for now!
#20
Looks good. Committed to CVS HEAD.
#21
Automatically closed -- issue fixed for 2 weeks with no activity.