Coder concatenates strings in odd ways during parsing
| Project: | Coder |
| Version: | 6.x-2.x-dev |
| Component: | Code |
| Category: | bug report |
| Priority: | critical |
| Assigned: | douggreen |
| Status: | needs review |
In _coder_read_and_parse_file, coder concatenates different strings on the same line into a single string. If the strings are indeed concatenated in php, this is helpful, however if the strings are simply two different string arguments, the concatenation introduces bad data into the quote processing rules. For example:
db_result(db_query("SELECT filename FROM {system} WHERE name = '%s'", "ad_$detail->adtype"));Results in the parsed string
SELECT filename FROM {system} WHERE name = '%s'ad_$detail->adtypeObviously, for the SELECT security checks, this should be treated as two strings and not one. So concatenating these two strings together is the wrong this to do.
The attached patch attempts to fix this. It turns all of the parsed #lines into arrays, so that you can have multiple values on a single line. And then every test needs to loop through the $lines values. I've fixed up all of the tests that are part of coder. But this is an API change, and will affect anyone who has written an external review.
As this introduces an API change, does that mean that we should automatically increase the version from 6.x-1.x to 6.x-2.x? I think so, but I'm asking for some feedback.
This patch passes all tests, even some of the new ones, that had previously failed (such as the example above).
One more note, we should review all the quote and doublequote rules to see if this now improves them, and if we should write additional tests that take this scenerio into consideration (that is tests with extra strings, separated by something.
| Attachment | Size |
|---|---|
| quote.patch | 11.69 KB |
#1
That looks good to me. I do think we need to increase the version number to 2.x since the API is changing. We should also create a task in the potx module issue queue requesting that it adds support for the new api. Not aware of any other modules which have implemented an external review.
#2
Here's a slightly updated version, that I think makes it so that the existing potx review doesn't need to change. The new indexes in $coder_args now have the names:
+ * '#all_array_lines', '#php_array_lines', '#allphp_array_lines',+ * '#html_array_lines', '#quote_array_lines', '#doublequote_array_lines',
+ * '#comment_array_lines',
But I've left #all_lines because this is helpful for some of the error reporting. #all_lines is the index used by the potx review, so I think that this will make it so that review doesn't need to change. We can still increase the version to 2.x to indicate strongly that there is an API change here. But my hope is that the API change doesn't affect the one existing third party review.
#3
Committed.
Because 'd like to start working towards a 6.x-2.0 release soon, probably as soon as the i18n review and potx reviews are stabilized, It would help to confirm that the current potx review works properly with this branch.
#4
I just ran the latest 2.x version with the latest potx code and it seems to work fine.
#5
Hmmm it doesn't play nice with SimpleTest however. The following 2 errors are repeated a lot in the SimpleTest results:
Undefined offset: 1 Notice coder.module 1159 _coder_read_and_parse_file()Undefined offset: 1 Notice coder.module 1161 _coder_read_and_parse_file()
#6
It also causes problems with the upgrade to D6 rules. The following errors are repeated over and over:
preg_match() expects parameter 2 to be string, array given in /Users/stella/src/sony/trunk/modules/contrib-dev/coder/includes/coder_6x.inc on line 501.preg_match() expects parameter 2 to be string, array given in /Users/stella/src/sony/trunk/modules/contrib-dev/coder/includes/coder_6x.inc on line 508.
#7
I checked in two changes that "may" fix this. Please try the latest dev release.
#8
Yep, all of the above problems have been fixed.
Cheers,
Stella
#9
Thanks
#10
Automatically closed -- issue fixed for two weeks with no activity.
#11
There's something wrong with this still, I think.
array(
'#case-sensitive' => TRUE,
'#source' => 'quote',
'#type' => 'callback',
'#value' => '_coder_tough_love_sentence_style',
),
function _coder_tough_love_sentence_style(&$coder_args, $review, $rule, $lines, &$results) {
print_r($lines);
}
prints:
[24] => Array
(
[0] => page callbackdrupal_get_form
)
[25] => Array
(
[0] => coder_tough_love_settings
[1] => page arguments
)
which are two oddities from these lines of code:
'page callback' => 'drupal_get_form','page arguments' => array('coder_tough_love_settings'),
Now, granted, a "=>" isn't a "concatenation", but it's being treated as a single string (which is, roughly, what this particular issue was meant to resolve). And, I'm not entirely sure why, in the second example, the second string ("coder_tough_love_settings") shows up BEFORE the first string ("page arguments").
#12
Attached is a patch to fix the => issue (but NOT the flipping of the page arguments example above) - it's the same as the patch I provided for Drupal 5 (which was wontfix'd in preference to this issue, but was apparently an entirely different problem anyways).
[24] => Array(
[0] => page callback drupal_get_form
)
#13