Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
Comment | File | Size | Author |
---|---|---|---|
#18 | 880278-cleanup_locale_inc-4.patch | 12.04 KB | RoboPhred |
#15 | 880278-cleanup_locale_inc-3.patch | 12.04 KB | RoboPhred |
#12 | 880278-cleanup_locale_inc-2.patch | 12.33 KB | RoboPhred |
#7 | 880278-cleanup_locale_inc-2.patch | 1.59 KB | RoboPhred |
#5 | 880278-cleanup_locale_inc.patch | 2.35 KB | RoboPhred |
Comments
Comment #1
andypostTagging
Comment #2
dstolOnce #860154: The locale module does not import the plural forms from the compact files is committed I'll clean up the function.
Comment #3
RoboPhred CreditAttribution: RoboPhred commentedAssuming 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.
Comment #5
RoboPhred CreditAttribution: RoboPhred commentedPatch not from drupal root (Thanks Heline).
Comment #7
RoboPhred CreditAttribution: RoboPhred commentedAbandoning 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.
Comment #8
RoboPhred CreditAttribution: RoboPhred commentedComment #10
RoboPhred CreditAttribution: RoboPhred commented#7: 880278-cleanup_locale_inc-2.patch queued for re-testing.
Comment #11
andypostGreat! But comments should be on new line - http://drupal.org/node/1354#inline
Comment #12
RoboPhred CreditAttribution: RoboPhred commentedThings 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
would become
Again, this is not part of the current patch. I am waiting for feedback on it.
Comment #14
andypostInteresting thoughts... maybe file another issue about?
4 tests are failed you touch only comments so what's wrong?
Suppose this conversion caused broken tests
Patch contains a lot of trailing white-spaces.
There's 2 spaces before 'Can'
Powered by Dreditor.
Comment #15
RoboPhred CreditAttribution: RoboPhred commentedI 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.
Comment #16
RoboPhred CreditAttribution: RoboPhred commentedComment #18
RoboPhred CreditAttribution: RoboPhred commentedMissed 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.
Comment #19
andypostLooks good for now!
Comment #20
Dries CreditAttribution: Dries commentedLooks good. Committed to CVS HEAD.