Array references - performance

Our performance discussions have been very suspect so far. See http://drupal.org/node/121388. Please reference your source for the 30x faster claim. Before adding more rules like this into coder, I'm going to ask for more community support. So if it's something you feel strongly about, please try to get a few +1's on this thread. Thanks!

http://drupal.org/node/175160

Apart from performance, it is simply bad coding. From the Gospel itself:

Why is $foo[bar] wrong?

You should always use quotes around a string literal array index.

<?php
  $foo[bar] = 'enemy';
  echo $foo[bar];
  // etc
?>

This is wrong, but it works. Then, why is it wrong? The reason is that this code has an undefined constant (bar) rather than a string ('bar' - notice the quotes), and PHP may in future define constants which, unfortunately for your code, have the same name. It works because PHP automatically converts a bare string (an unquoted string which does not correspond to any known symbol) into a string which contains the bare string. For instance, if there is no defined constant named bar, then PHP will substitute in the string 'bar' and use that.

Nancy asked me to look at this. I'm still pretty green with PHP but I think I understood what Heine posted. So if this is slower and is even bad coding, sounds to me like a good thing for the coder module to be flagging.

Michelle

Heine convinced me.

Good. I'm wondering how many people are going to get flagged on this. I know that the one site I work on will have beaucoups flags. But performance will vastly improve.

Nancy, thanks for bringing this up, looks like it might bring some useful performance benefits. I'm not in a position to run these sorts of tests on the sites I manage, but it will be interesting to see from others just what sort of speed increases we will have. If its really significant, then there are going to be some heavy patches coming down the line :)

Thanks for bringing this up. I added a rule to check for this today. Sorry it took so long, but it's been a crazy month. I just came up for air yesterday :)

No problem. You did it faster than many maintainers. Thanks for adding thsi.

This test appears to have one bug: it ignores named constants, like:

In this case, the warning seems to be unduly triggered, because (I think) the current regexp mechanism cannot be made aware of the fact that this is actually a valid constant, and not the semi-erroneous case discussed above.

Would this work out if we would check case-sensitive? Of course, constants can also be in lowercase or mixed case which would be totally valid. However, most constants are in uppercase -- so a regexp [^A-Z] could work out better.

That's a good idea. This patch should catch a few more problems. Can someone test it please?

Patch does not address the problem I raised in #10.

Still, since coder actually reads the files, maybe it will have such constants available from get_defined_constants() and could check whether preg matches are present in the list of defined constants and remove the warning in that case ?

I don't think that we can use get_defined_constants, because coder don't process php's requires and includes directives, it only reads the files with certain extensions, within the module's directory structure.

The patch in #12 should ignore upper case constants, if that's not the case, I think that we simply need to fix the regular expression or the #not processing.

There was a bug in the coder #not processing. This patch should work.

Regarding named constants, coder does not process the directives, as you say, but if the module is enabled it should parsed in the normal drupal run cycle, its globals and symbols are available to coder just like they are to other modules. I just checked it by dumping that list from within coder_style reviews().

Yet better than the enumeration, which can be heavy in RAM, it is possible to simply use "defined(theconstant)" to check whether the constant is defined or not, and this would also work for class constants like Foo::BAR.

@fgm, coder runs on modules that aren't even enabled, so again, I don't think that we can use get_defined_constants.

@doug: indeed, it *can* run against non-enabled modules, but it can also run against enabled modules, in which case the symbols are actually available and this test will work.

@fgm, you're assuming that an enabled module will include all it's files when the module itself is included. This is a falacious assumption. Many modules include files only when certain functions get called.

We'd have to have a conditional check, providing one set of rules for enabled modules and another set for disabled ones, and since the rule for disabled ones would still not be guaranteed correct, we'd end up falling back to the regex at times. I don't think that the extra code provides us much. If you feel strongly about it, please create a patch, but I'm skeptical.

Coder relies on many coding standards and constants as capitals is a pretty universal coding standard. I don't understand why you're suggesting the extra work. It's would actually be a different approach than coder takes for every other rule.

@dougreen: "I don't understand why you're suggesting the extra work" : simply because in the current situation, some warnings are actually wrong and can cause users of coder.module to lose time wondering why the module claims they are using unquoted constants when in fact they are not.

Seeing how coder.module works, it obviously makes sense to have this error appear, but from the module user's POV, it is very counter-intuitive : the module is a code quality feature and yet it throws incorrect warnings when developers use best current practices (defined constants instead of inline strings). Which image does that project about the overall drupal quality ? IMHO, not as great an image as we would all wish.

@fgm, if the module developer follows code standards and uses upper case constants, there should be no false positives. Sorry for being so dense here, but I believe that this patch does properly not catch the problem in #10, but does catch the problem if they use lower case or mixed case text. Please give me an example.

I've tested this and I think it's good to go.

Cheers,
Stella

Stella, since you've tested it, can you also commit it? Thanks!

Committed.

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