Posted by stella on September 21, 2008 at 7:20pm
| Project: | Coder |
| Version: | 6.x-1.x-dev |
| Component: | Review/Rules |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
Issue Summary
Hi,
A false positive is generated for lines like:
'titleUser' => t('Your rating') .': ',The error given is "The $string argument to t() should not begin or end with a space.".
The t() string doesn't contain a space, but the string to which it is concatenated to, does. Issue found in code review of fivestar.module, lines 1420 and 1421.
Comments
#1
Attached patch should fix this issue.
I could have changed the regex to:
- '#value' => '[\s\(]t\s*\(\s*[\'"](\s+|.*\s+[\'"]\s*[,\)])',+ '#value' => '[\s\(]t\s*\(\s*[\'"](\s+|[^\'"]*\s+[\'"]\s*[,\)])',
However this would then miss trailing spaces in the string if the string itself contained a single or double quote. I've tried to over come this scenario by changing the rule to:
array('#type' => 'regex',
'#value' => '[\s\(]t\s*\(\s*[\'"](\s+|.*\s+[\'"]\s*[,\)])',
+ '#never' => '[\s\(]t\s*\(\s*[\'"](\s+|([\'"]\s+[^,\)])*.*[^\s][\'"][,\)])',
'#source' => 'allphp',
'#warning_callback' => '_coder_i18n_space_starts_or_ends_t',
),
This should do the current match, but should then exclude matches where there is a quote followed by a space and something which is not a comma/bracket and which is also terminated by a quote and a comma/bracket which isn't prepended by a space. Hope that makes sense.
So the following checks should now be alerted upon:
$var = t('This isn't a false positive ');$var = t('This isn\'t a "false" positive ');
but won't alert on:
$var = t('This is a false positive') . implode(', ', $array); // This is the one that is currently happening, frequently.$var = t('This is a \'false\" positive');
$var = t('This is a "false" positive');
I'm pretty confident this works, but I'd like it if someone could test it and confirm.
Cheers,
Stella
#2
This fails two tests.
#3
Ok, here's a slightly modified one. It catches all 6 tests.
Cheers,
Stella
#4
#5
Thanks! Committed. BTW, Coder is now passes all coder tests (although we still need to convert many :)
#6
Hmmm, this might need more work. It's now not catching lines like these:
$description[] = t('!days', array('!days' => implode(t(' and '), $results)));$description[] = t('until !until ', array('!until' => date_format_date($until, 'custom', $format)));
$description[] = t('except !dates ', array('!dates' => implode(', ', $values)));
#7
The first patch has already been committed, so this one is in addition to the above one. It should now catch most of the above scenarios. It catches all but the first one, where there is a nested t(). I'm not sure how to change the code to catch this, or even is something we should be trying to handle. Are nested t()s really valid? I'm inclined to think not.
The other scenario where it may not work is:
$var = t('Your rating') . t('(fail) ');i.e. where there is more than one t() call on the same line, and the last one has a trailing space and the last one also contains a closing ). The chances of this are probably slim enough, though it could probably be fixed with a bit more effort. I do think this patch is an improvement in any case - just try some of the new test cases with the old rule to see.
Oh it also checks for leading/trailing spaces in get_t() and st() too.
Cheers,
Stella
#8
Committed, I added one test to your additional tests. Thanks!
#9
Automatically closed -- issue fixed for two weeks with no activity.