By dman on

A general PHP question.
I do most of my dev on a HEAD checkout these days, which is freaking anal about PHP strict notices.
I try to tidy them up as I go, because ignoring a hundred boring ones means that I may miss one useful one. Usually the fixes are trivial. But occasionally create horrible code.

Todays challenge:
- from taxonomy_xml XML parser
- adding found elements to a structure,
- building an array on the fly,
- concatenating strings if there are more than one

Found some data, $_tx_term and $_tx_tag are context... it's possible nothing is initialized.

        $_tx_terms[$_tx_term][$_tx_tag] .= trim($data);

Simple. Worked.
With strict, I get a million notices about

# notice: Undefined offset: 44 in ../xml_format.inc on line 114.
# notice: Undefined index: synonyms in ..xml_format.inc on line 114.

Do I really have to replace that one-liner with half a page of "if is_set() then else..." crud?
Even the .= needs to be replaced with 

if (is_set($_tx_terms[$_tx_term][$_tx_tag]) {
  $_tx_terms[$_tx_term][$_tx_tag] .= trim($data);
}
else {
  $_tx_terms[$_tx_term][$_tx_tag] = trim($data);
}

!! The ternary version doesn't look any better

Anyone know a shortcut? Better syntax?

... On a whim, I find that I can disable notices for JUST THIS LINE using @ . I never knew it could work for assignments, not just function calls!

        @$_tx_terms[$_tx_term][$_tx_tag] .= trim($data);

Anyone have any suggestions? Shall I just bang a @ in front of future strict notices I don't care about? Or is that being a bit ON ERROR CONTINUE about it?

Comments

tmallen’s picture

tmallen commented

Ouch, suppressing a ton of errors will grind your script to a halt. Obviously it's also a performance hit to look up non-existent array offsets, so you're best off either checking with isset($array[$offset]) or by making sure ahead of time that only valid array keys will be included.

dman’s picture

dman commented

Are you saying that suppressing - ignoring - this notice has a performance hit? Surely ignoring them is even cheaper than reporting them? Especially coming through the Drupal error-handler.
Is the cost of discarding it greater than adding a whole bunch of isset() logic inside this tight loop?

Yes, the lookup on a non-existent index isn't free, but surely it's not going to be more expensive than a lookup on an existing one?
And internally, PHP core knows how to instantiate it on-the-fly (which it does, and is what I want) and that's GOTTA be more efficient than me doing exactly the same job at the interpreted level.
I don't second-guess a lot about PHP internals, performance-wise, but I don't think that me doing in a page of code what the system was otherwise doing just fine on its own can be good. All I'm seeing is a notice when it does so, which is boring. On a live site, of course, it would be inappropriate to have error_level 'notice' enabled.

I know how it could be replaced, but my big problem is that the replacement for the one-liner is

- does index A exist? If not make it
- does index A,B exist? If not make it
- does string at A,B exist? If not make it
- finally, concatenate the string.

And that's a heap of code to replace the current one action. :-/
And this is a very tight loop that gets called a few hundred times.

.dan.
if you are asking a question you think should be documented, please provide a link to the handbook where you think the answer should be found.
| http://www.coders.co.nz/ |

.dan. is the New Zealand Drupal Developer working on Government Web Standards

gforce301’s picture

gforce301 commented

Actually the performance hit is slightly more to ignore a warning than to show it. I know this sounds odd but the warning is generated even if it is suppressed or not and that in and of itself is a performance hit.

The suppression operator '@' also adds a very slight performance hit itself, making the suppression of a warning even more (if only slightly) costly.

The bottom line is, only true execution benchmarking of the code would tell you for sure if extending the logic to remove the warnings would cost more than leaving them. Thats why IMHO it is always best practice to write warning free code unless it is proven that code with warnings will execute faster.

Here is a link to an article discussing the performance hit of warnings and of suppressing them --> http://vega.rd.no/article/php-performance-error-suppression

dman’s picture

dman commented

I wanted to do an actual benchmark just to see what I could see, but ended up going down the rabbit-hole of installing xdebug and configuring it right, then looking for a cachegrind equivalent for my macbook and messing round with repositories and fink and .. just arrg.
:-/

.dan.
if you are asking a question you think should be documented, please provide a link to the handbook where you think the answer should be found.
| http://www.coders.co.nz/ |

.dan. is the New Zealand Drupal Developer working on Government Web Standards

cog.rusty’s picture

cog.rusty commented

The @ (pardon me the expression) looks like cheating to me.

From what I understand (and I may be wrong), the point of the E_STRICT hunt is not performance or convenience but making PHP code look more like other, more "disciplined" languages. In most languages that I have met, an undefined variable can never enter the code and get away with it. Usually there is no is_set() and the program is laid out in such a way that any variable has been initialized first, even with an empty or a special value, and any index range has been handled.

I am in no way experienced with PHP (well, I can write 10 lines an hour), but maybe your code snippet comes in too late for handling it with that kind of "discipline", because the logic is already set. On the other hand, I can see the convenience.

dman’s picture

dman commented

I agree that in my gut, @ looks like a cheat, but I think the point of E_STRICT is to draw attention to the code and say "are you sure this is right?". PHP is softly typed, and a few of these features make simpler, and in this case better, code. It looks like replacing it with a long tedious alternative is silly.

To compare it to my only other conscious use of @ - 

  $url_parts = @parse_url($input);
  if ($url_parts['host']) { 
    // do some URL processing
  } // else Doesn't look like an URL

The @ is there because I get a notice when the parse fails.
I KNOW - that's why I tried the parse in the first place.
There's no way I should replace the built-in php URL parser with my own messy version, but the notice makes it look bad.
It's not bad, I know what I'm doing, and it's right.
So I'm justified to suppress it.
I think I'll try using that logic for this case too...

.dan.
if you are asking a question you think should be documented, please provide a link to the handbook where you think the answer should be found.
| http://www.coders.co.nz/ |

.dan. is the New Zealand Drupal Developer working on Government Web Standards