2

I'm running through someone else's code, and they constantly avoid escaping their array keys.

For example:

$row_rsCatalogsItems[Name]

instead of

$row_rsCatalogsItems['Name']

so I'm constantly making tiny changes to everything I touch just to deal with that bit of laziness. But now I'm wondering whether there's even much benefit to doing so. I get that it'll check for constants before defaulting to a string (I detest that behavior in php when dealing with constants, since they validate to true even if not defined), but I'm not sure even that is worth the bother of changing all the many, many instances of constants-as-array-keys.

Suggestions?

Kzqai
  • 22,588
  • 25
  • 105
  • 137
  • I think you should bite the bullet and plan on changing them to quoted. Normal refactoring tools may not help but this doesn't mean you can't try hacking around it rather than editing them all by hand :) Perhaps a find/replace with regular expressions, anything that is in a [] and doesn't start with " or ' or $: – Fanis Hatzidakis Apr 12 '11 at 22:26
  • Having taken the arguments herein to heart, I decided to start this thread: http://stackoverflow.com/questions/5642483/method-e-g-via-bash-script-to-turn-php-array-indexes-currently-using-constants to try to get a good bash script to automate the process going. – Kzqai Apr 12 '11 at 22:45

3 Answers3

9

There is more than one good reason.

Do

error_reporting(E_ALL);

in the beginning of your script and you will see all the reasons at once.

Fun aside, in practice you might think that well, this is something that I might get away with. And you could be right. But as a developer you have to draw the line somewhere about what is an acceptable hack and what is reason for verbal abuse. For me, this particular behavior is beyond that line.

The very immediate reason why this is bad is that it makes error_reporting(E_ALL) unusable. And good development practice demands that all errors are reported, these notices are setting you up for buggier code and harder debugging sessions.

Update: I didn't address the issue of a practical solution to the existing situation, so here's what I would do in your shoes:

  1. Find the person responsible and make sure that they never, ever do this again by any means necessary.
  2. Run a problematic script and get all the notices about undefined constants from the log file.
  3. Using regex search and replace from your editor, try to replace \[(a-zA-Z_-)\] to ['$1'] (or something similar). If the number of replacements is equal to the number of notices in the log file, you are golden. Otherwise, try divide and conquer until you see where the regex is failing.
  4. Repeat as required for all the other scripts.
Jon
  • 428,835
  • 81
  • 738
  • 806
  • Definitely. It's not so much "escaping" as "properly using string literals instead of the default behavior of undefined constants" – Dereleased Apr 12 '11 at 22:13
  • @Dereleased: Definitely. The OP seems to understand how it works too; he's asking for practical advice. – Jon Apr 12 '11 at 22:15
  • Absolutely. Plus, fixing such "minor" issues may pay off further down the road in helping avoid mistakes. Consider what will happen if months later, in a different included file, that unquoted string is defined as a constant: `define('Name', 'something else')` – Fanis Hatzidakis Apr 12 '11 at 22:19
  • I did turn on all errors and notices for a moment, just for fun, and the result was effectively unusable. Which is the problem; my time is precious, and the code atrocious. These constants are just the tip of the iceberg, so I'm trying to figure out if there's a performance gain or a potential for horrible namespace clash with existing defined constants or even overhead on the propegation of notices even if they're turned off, nasty little things like that. – Kzqai Apr 12 '11 at 22:20
  • @Tchalvak: As you saw yourself, the biggest problem with this code is that *it forces you to run half-blind* (there might be a notice in there you want to see, but it's unusable as you said). Take a look at the update for a practical suggestion. – Jon Apr 12 '11 at 22:25
  • 1
    I guess the moral of your post is that it'll *never* be possible to work with notices on while using the code at this rate, which is a good point. Darn, back to adding lots of ' back in. – Kzqai Apr 12 '11 at 22:26
  • @Tchalvak: Definitely. I 'll put this up into the answer. – Jon Apr 12 '11 at 22:30
  • 1
    This `((\$.*?)\[([^'"$\d].*?)\])` could work as a starting point to match all array key invocations where the key doesn't start with ' " or $ or a number (array index). Then \3 could be replaced with "\3" – Fanis Hatzidakis Apr 12 '11 at 22:35
  • Also remember that just because errors are not displayed does not mean they are not raised, so having these errors does still result in a performance hit. – Dereleased Apr 13 '11 at 20:22
  • by the way, the correct php syntax for an array -in a string-, e.g. "this is a string $someArray[key1] with an array key in it" is actually without quotes, which makes it really hard to run a regex replace on it. *sighs* – Kzqai Nov 22 '11 at 16:45
2

It's not technically correct to do it that way. You should probably use single quotes. It will use less cpu cycles and memory to run the code if you fix those. Will you notice a difference even if there are 1000 of those fixed in one file? Probably not.

If you fix it, it would be for correctness and not having warnings or notices, not really for speed or memory usage.

profitphp
  • 8,104
  • 2
  • 28
  • 21
1

I'd add the quotes. If nothing else, to ensure that:

  1. Code readability isn't reduced (which it is if someone thinks you're using some constant rather than a string value)
  2. Extra cycles aren't wasted on each evaluation

Having said that, I understand that a programmer's time is valuable. Therefore, you can optionally out-lazy the lazy coder and use a regular expression like the following:

$input = '$row_rsCatalogsItems[Name]';
$str = preg_replace('/(\$[a-zA-Z_]+\[)([a-zA-Z]+)(\])/', '${1}\'${2}\'${3}', $input);
echo $str;

(You might want to step through the changes using that expression just to make sure you're changing the right things ;))

Demian Brecht
  • 21,135
  • 5
  • 42
  • 46
  • I'll probably end up having to use bash scripting on the script files (in git, so it's reversible), which is probably a good topic for a totally different question. – Kzqai Apr 12 '11 at 22:29
  • I don't actually get how that regex would be used. Are you saying I should use that script with .php scripts as input? Or are you positing it's use for `eval()`'ed code or something. – Kzqai Apr 12 '11 at 22:42
  • Sorry, should have clarified - any editor should be able to take a regex in a search/replace (I just used php to test it). Alternatively, you could write a script to go through all source files and apply the rule (breaking for confirmation or not). At the end of the day, either way your time is saved from doing it manually. – Demian Brecht Apr 12 '11 at 23:15