Check for unquoted strings that aren't defined constants
NancyDru - June 24, 2008 - 16:16
| Project: | Coder |
| Version: | 6.x-1.x-dev |
| Component: | Code |
| Category: | feature request |
| Priority: | minor |
| Assigned: | Unassigned |
| Status: | active |
Jump to:
Description
Is there any way that Coder can catch things like the missing quotes in drupal_get_form(mymodule_settings_page)? Yes, I know it was a stupid mistake. I thought we had visited this issue once upon a time.
#1
This could actually be a valid line if you had a
define()for mymodule_settings_page. I think that the discussion we had once upon a time was about the strings (and defines) as it related to some other rule. However, it is a standard for defines to be upper case. So in this case, I think we could catch it with a rule that looks for'regex' => 'drupal_get_form\([a-z_]+\s*[,\)]', 'not' => 'drupal_get_form\([a-z]+\s*\(',... regex's are untested.#2
Well, the general case problem is for ALL function calls or unquoted strings anywhere unexpected to raise warnings, I don't see what's special about drupal_get_form. So that means paranoia everywhere.
So this becomes a PHP parsing thing. I guess log level notice may show some of that.
Or, what I do is use a proper IDE that does syntax highlighting and helps locate issues like that. I use Eclipse which subtly indicates when I'm using an undefined constant or variable.
#3
I think dman is right, there is nothing special about drupal_get_form. That just happens to be where I messed up this time. It may or may not have been logged; I didn't look (so spank me). My syntax highlighting was also insufficient for me to catch it.
#4
@dman, good point, as it sounds like the consensus is that this is a pretty special case, I'm marking as "won't fix."
#5
Pardon me for re-opening this, but I believe the issue is not the drupal_get_form, but unquoted strings. That problem does need to be fixed. I thought it was, once upon a time.
#6
Unquoted stings are legal in PHP. Although they just get cast to literal strings ... which is almost never what you want.
But constants for example are fine.
I think that the coder module is going to have a hard time telling the difference between
<?phpprint(IMAGE_ORIGINAL); // A constant defined elsewhere, a long way away.
print(IMAGE_MEDIUM); // An unquoted string? Should it raise an error? Or was it defined somewhere already?
?>
Or do you think it's possible to come up with an expression that can detect unexpected unquoted stings - that are NOT function names, constants, variables or PHP keywords - better than the PHP parser itself can?
<?phpprint("an example 'hook' is <pre> hook_example(arg1) </pre>"); // this line should not trigger any problem
drupal_set_message('call_me'. call_me_too); // this line is a problem
drupal_set_message('call_me'. $call_me_three); // this isn't
$output = "Incorrect strings may concievably be ". anywhere . in . the . code;
$callback_func = my_callback; // and they are not always wrong. This line may be legal.
for (){}; // All PHP constructs may look like just unquoted strings unless we keep a list.
call_func(true); // It's a lot of work for a regexp to know that this is OK
call_func(maybe); // and this is (maybe) not
define('maybe', 0.5); // and now it is. Although not exactly Drupal-compliant still.
?>
... etc. Yikes.
:-(
It's really a PHP syntax tokenizer job. I think the parser does raise notices if the volume is turned right up.
#7
Here's the previous issue: http://drupal.org/node/180226. While it was about array references and performance, someone pointed out that while PHP allows unquoted strings, it also considers them "wrong." I haven't done any kind of benchmark on this like I did on the other issue, but it is still wrong, and undoubtedly quite slow due to error handling.
#8
@dman, if we wanted to work with this, I think that we could rely on the standard that defined constants are UPPERCASE.
@NancyDru, But I think that the number of use-cases to check will be very hard to do.
#9
@dougreen, yes, if nothing else, non-upper constants are bad drupal-style anyway, and already raise warnings.
That previous thread makes sense, as using unquoted array keys is a known (although discouraged) practice.
Also it would have still worked as expected, and does come down to a style and best-practice choice. Including efficiency of course.
Unquoted strings as in the OP example are much more rare ... in many languages anyway, and are more probably just code mistakes than stylistic choice.
In that discussion many of my misgivings are also addressed.
However, there's a difference in code style (discouraging a deprecated practice) and actual syntax checking broken code.
I also think that the regexp patterns needed to assure no false positives (if even possible) would be horrible. :-/
If someone want to demo a proof-of-concept for the general case, good luck, but it scares me.
#10
I absolutely accept that my case was just my being brain-dead at the time I typed that code.
Drupal's coding standards are as much, if not more, about coding style rather than broken code, so I don't see this as totally outside Coder's purview. However, I understand if the checking is too difficult. At least the issue is understood.