tags:

views:

64

answers:

4

I have turned on error_reporting(E_ALL) and run this code:

$topic_id = (int) safe_query($_GET['top_id']);

if($topic_id > 0)
    include("topic.php");

And get this error: Notice: Undefined index: top_id. Is it that bad what I do? If yes then why? Should I check if $_GET['top_id'] isn't empty before I give its value to $topic_id? Why? Thank you.

A: 

I understand the question now. I'd suggest using isset just to be safe. I'm assuming you don't have a topic_id that is 0.

Rocket
Because if it doesn't exists, it just becomes zero and it's ok for me. It works, so why should I change it, that's what I am asking.
hey
A: 

It's not really a problem here, since you only take action on it if it's set anyway. However, you're just lucky that the unset value evaluates to false. Also, it'd be annoying to continue having it give you the warning. You'd probably be best suited by doing what others have suggested, checking if it's set before using it. It might be easiest to just set up a function that does that, especially if you're going to be checking a lot of GET parameters.

Whatever you do, don't just turn the warning level down to suppress the warning; in this case, it doesn't hurt to assign from the unset variable, but in the future it could indicate an actual error.

Doug Kavendek
+3  A: 

One of the reasons why I do it is to prevent unexpected behaviours.

Code should always reflect the intention of the programmer. If a behaviour depends on some mysterious process in the background, eventually it will come and bite you in the ass when you are knee deep within bugs and debugging.

Traditionally, attempt to access an array with a key that doesn't exist causes a crash (probably in unmanaged environment) or an error. PHP silently 'fixing' this in background is great for beginners, but bad for debugging. Your code will work, but may give you unexpected result.

Take for example, your code. Say the calling page forget to specify the top_id, or misspelt it as topid, and PHP goes on its merry way. It didn't include topic.php, and nothing happens. The code works fine. PHP doesn't complain. What's wrong?

Now, your code is short. What happens when it is longer? Nested deep within many lines, between different functionalities? For your case it isn't a big deal, but when doing complex array manipulation it will make debugging harder.

Extrakun
A: 

"And get this error ... Is it that bad what I do?"

Well, it's not giving you an Error. It's giving you a Notice. Notices are "ignored" (that is, not echoed) on production servers.

Essentially, PHP is telling you what Extrakun is saying in his answer. You should take notice to a potential mistake that could lead to a real error later.

So, "Is it that bad what I do?" ... maybe not. But, then again, PHP is also not giving you an error. It is giving you the right amount attention that the code segment deserves - a Notice.

stjowa