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->adtype

Obviously, 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.

CommentFileSizeAuthor
#17 coder-314268-17.patch549 bytesdanepowell
#12 spaces_MmMm.patch727 bytesmorbus iff
#2 314268.patch14.02 KBdouggreen
quote.patch11.69 KBdouggreen

Comments

stella’s picture

stella commented
Status: Needs review » Reviewed & tested by the community

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.

douggreen’s picture

douggreen commented
StatusFileSize
new 314268.patch14.02 KB

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.

douggreen’s picture

douggreen commented
Version: 6.x-1.x-dev » 6.x-2.x-dev
Status: Reviewed & tested by the community » Fixed

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.

stella’s picture

stella commented

I just ran the latest 2.x version with the latest potx code and it seems to work fine.

stella’s picture

stella commented
Status: Fixed » Needs work

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()
stella’s picture

stella commented

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.
douggreen’s picture

douggreen commented
Status: Needs work » Needs review

I checked in two changes that "may" fix this. Please try the latest dev release.

stella’s picture

stella commented
Status: Needs review » Reviewed & tested by the community

Yep, all of the above problems have been fixed.

Cheers,
Stella

douggreen’s picture

douggreen commented
Status: Reviewed & tested by the community » Fixed

Thanks

Anonymous’s picture

Anonymous (not verified) commented
Status: Fixed » Closed (fixed)

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

morbus iff’s picture

morbus iff
they/them
commented
Status: Closed (fixed) » Active

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").

morbus iff’s picture

morbus iff
they/them
commented
StatusFileSize
new spaces_MmMm.patch727 bytes

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 
        )
morbus iff’s picture

morbus iff
they/them
commented
Status: Active » Needs review
stella’s picture

stella commented

Do you know if this is still a problem with latest 2.x branch?

stella’s picture

stella commented
Status: Needs review » Fixed

Ok marking as fixed then. Feel free to reopen if there's still an issue.

Status: Fixed » Closed (fixed)

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

danepowell’s picture

danepowell commented
Priority: Critical » Major
Status: Closed (fixed) » Needs review
StatusFileSize
new coder-314268-17.patch549 bytes

I don't know how this has gone unnoticed for so long, but it's definitely still an issue. The patch in #12 still works, it just needed to be rerolled (see attached).

douggreen’s picture

douggreen commented
Version: 6.x-2.x-dev » 7.x-2.x-dev
douggreen’s picture

douggreen commented
Status: Needs review » Postponed (maintainer needs more info)

I don't see the problem. Take the following test file:

<?php
$a = 'foo' . t('bar');
$b = 'foo' . 'bar';
$c = array('foo' => 'bar');

Then the output is

Array
(
    [2] => Array
        (
            [0] => foo
            [1] => bar
        )

    [3] => Array
        (
            [0] => foobar
        )

    [4] => Array
        (
            [0] => foo
            [1] => bar
        )

)

In Line 3, which is two strings concatenated together (really), coder seems to handle this correctly (AFAICT).

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Postponed (maintainer needs more info) » Closed (won't fix)

Coder 7.x is frozen now and will not receive updates. Coder 8.x-2.x can be used to check code for any Drupal version, Coder 8.x-2.x also supports the phpcbf command to automatically fix conding standard errors. Please check if this issue is still relevant and reopen against that version if necessary.