False suggestion: table names should be enclosed in {curly_brackets}

Yes, I also get the suggestion when the query string is composed of concatenated variables, which do include curly brackets. See attached. I think this is a false positive, which according to the principles by which Coder is developed, are intended to be avoided. ie it is better to let an error pass than to falsely suggest to make a change which is wrong. I had the case when someone followed the advice and created a bug by adding { } which were already there. Admittedly, they should have known better, but it would still be good if Coder could be improved to only give this suggestion when the string is a plain sql statement.

Jonathan

Lost the attachment. Here it is.

i get the same error message on a line where a table name is in curly brackets:

      $query = "SELECT cid FROM {comments} WHERE nid = %d AND cid <> %d AND LOCATE(TRIM(TRAILING '/' FROM thread), '%s') = 1 ORDER BY thread DESC";

garbanzito, your false positive occurs because of the second occurrence of FROM in the string being followed by a word without braces.

I ran in this problem too. Not a biggy since we can just ignore false positive...

In my case, though, the string appears in a t() macro.

From what I've seen, at this point all you do is parse the code using regular expressions, so at this point I don't really know what you could do about this...

Thank you.
Alexis Wilke

I have been getting this issue on numerous t('string') calls such as :

Line 1519: table names should be enclosed in {curly_brackets}
'#description' => t('Select the team from which this player should be removed.'),

This looks to be an easy fix, by just setting to exclude the t function on sql checking in my case. However could not get that to work.

I Just add this:
'#function-not' => array('t'),

however this did not work???

so as a real kludge, I exclude quoted strings as being sql if they start with "The" or if they have " the " in the string. Not foolproof and not real exact but works to get rid of annoying times it thinks it is sql just because the string starts with "select". And most english sentences or phrases have "the" in them, but sql doesnt'

so I added this:
'#not' => '^(The|.*\s+the\s+)',

patch is attached

As a matter of record only, this still happens:

Line 90: table names should be enclosed in {curly_brackets}
    '#title'         => t('Delete from whitelist when comment is deleted'),

Not a big issue though, of course.

Happening in the 7 version as well...

There's a patch here. Marking "needs review".

Trailing white space in patch, so needs work.

Doesn't seems like a good check to me: "SELECT * from node where title = 'the'" (the in sql), "Select something that matches." (no the)

no white, space and still not ideal, but not sure how to get it to exclude t function which is better solution. HOwever now doesn't annoy on t strings with select in them if also they have "the" in the string. Agree not a good check , but can't think of another way to exclude non-sql other than avoid the t function.

I got here after:

Line 192: table names should be enclosed in {curly_brackets}
'#description' => t('Select where the ShareThis widget should appear. When selected to display as a block, you must choose which region to display the ShareThis block in from the Blocks administration.', array('@blocksadmin' => url('admin/structure/block'))),

The git repository dosen't have the file includes/coder_sql.inc or the function coder_sql_reviews().

I'm not sure where I'm supposed to be using. Can't seem to grep or locate the code/files. The logic looks good (although rather simple).

Ok. I've just patched this against the git repository. Not sure why this wasn't coming up when I was searching for it.

EDIT: Oh ya, and it works. I applied it in a D7 environment I was working on.

Hi,
There must be a better way to detect that a statement is not sql, other than relying on the string to contain the word "the". Even though the patch may have worked for the cases above, there is no way it should be committed to the real coder source. I've not looked in detail at how coder detects or rejects strings for a given test, but it seems to have plenty of options, and we should be able to devise a better patch.
Jonathan

I agree.. It's pretty rough.. On the other hand, it's probably better than false errors.

agree with all that, think root issue is it seems to think if a string has "select" in it it is sql. If it had a better way to just deal with strings that are used in db_update and other sql api calls, then it would be a non-issue.

Also for me I was unable to exclude it checking string on a t function, which likely is not going to sql and is just a normal real string for display.

interesting, i'm working on Oracle database where I need to write some custom select statement where I starts to get this error.

We're not making progress here. This is a pain on qa.d.o, especially because coder_ignore isn't working either on qa.d.o.

The 'the' heuristic is not perfect, but it is a vast improvement over the current inability to get a clean run on qa.d.o. This is frustrating and demotivating. If we can't get Coder Review to work reasonably, it will continue to be ignored.

The 'the' heuristic is on the same level as the current rule, which completely ignores the context, leading to the false positives that we're seeing. If the current rule can blissfully ignore the context, then I don't see why the exception would need to go out of its way to check for t() or whatever.

Setting RTBC for #13 for D7.

I've already hardened the SQL SELECT check in 7.x-2.x by preventing it from looking inside t(). Could someone take a look at it and see which of the false positives listed on this page, are still false positives. I'm not totally opposed to the proposed solution to skip the word "The" (but I think the regex is a bit wrong ... I'm pretty sure that we don't need to check if it's at the beginning of the string).

I've tested the patch in #13 on all the examples in this thread. The code I used was:

/*1*/   $_business_contact_array = array('Select One', 'I buy/use Torani', 'I\'m on the mailing list', 'From my distributor');

/*2*/   $res_time = db_query('SELECT MAX(created) FROM ' . $sql_from . ' WHERE ' . $sql_where);
  
/*3*/   $query = "SELECT cid FROM {comments} WHERE nid = %d AND cid <> %d AND LOCATE(TRIM(TRAILING '/' FROM thread), '%s') = 1";
  
/*4*/   $in_t['#description'] = t('Select the team from which this player should be removed.');
  
/*5*/   $in_t['#title'] = t('Delete from whitelist when comment is deleted');
    
/*6*/   $string = "Select something that matches.";

/*7*/   $sql = "SELECT * from node where title = 'the'";

nb. Example 7 is the only one where the message to add curly brackets is applicable.

Here are the results BEFORE the patch:

This shows that examples 1 and 6 do not trigger a false positive.

AFTER the patch was applied:

This shows that the patch as it stands fixed example 4, but examples 2, 3 and 5 still got through with false positives.

Hope this helps. I will keep my test module and example code, and re-test any ammended patches.

Jonathan

I've just noticed that I patched the dev version of 1.1, dated 21st Sept. Is the test still valid? or do you want me to download 2.x and re-do it?

Please test against 7.x-2.x, I haven't hardened the version you tested.

Same issue, but reported for the (now unsupported) 6.x-2.x version: #315918: false positive on "select this and that from there" strings. Should that issue be closed as a duplicate?

You are right that #315918: false positive on "select this and that from there" strings is a duplicate, but Stella produced quite a simple neat patch and some enhanced tests on that issue.
Have those ideas been incorporated into the patches on this thread?

Jonathan

I'm experiencing this false positive with code:

$s = array('some_text' => array('type' => 'textfield', 'default' => t('Select the invoice(s) you wish to pay from the list below.')));

I changed string 'Selected the invoices ...' to 'Please select the invoices ...' and that fixed it ;)

I guess it always pays to be polite! :)

Except that Drupal's new policy is not to "please"...

The patch in #13 does not apply in 2.0 but I made the line change manually and tested.

Before

After

So the patch has fixed case 4 as it did before. Case 5 is now fixed by the new line

    '#never' => '[\s=\(](t|st|dt)\s*\(',

Case 7 is the only one which should have the warning, so it is just cases 2 and 3 which give the false positive.

Jonathan

The rule is to look for strings that start with the ^SELECT|, ^INSERT, or ^UPDATE, this is why adding "Please" before the string makes sure we don't trigger this check. Given that D7 has DBTNG we are likely to have more FALSE positives. Should the new rule be to look for ^INSERT or ^UPDATE and suggest that you use DBTNG? We could switch to only looking for the SQL keywords in CAPS, which is the standard anyway? And given DBTNG, we might drop the check altogether (this shouldn't happen too often anymore)?

Maybe even just flag db_query() and point to DBTNG?

Raw SQL can cause problems with databases like Oracle and SQL Server, which have common words (column names) reserved. The db drivers can handle that if it comes via DBTNG, but it's too cumbersome with raw SQL.

I love this module!!

I made the suggested edits and my errors disappeard

I posted my resolution here https://www.drupal.org/node/2456949 -- all I know is that I was not able to click on the "Memberships" link tab prior to. me editing those files .. which is the culprit being called out on those errors.

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.