Comparing undefined variables is not so good. Please consider that is_array() chokes on an undefined variable. Maybe your server needs this

error_reporting(E_ALL);
ini_set('display_errors', TRUE);
ini_set('display_startup_errors', TRUE);

in your settings.php to show you all PHP hickups?

CommentFileSizeAuthor
og_forum-php-notices.patch2.51 KBeMPee584

Comments

eMPee584’s picture

eMPee584 commented

for completeness, assigning undefined properties to variables then checking if they are defined is not so.. uhmm.. you get the idea ;)

dazweeja’s picture

dazweeja commented

Fair enough, my bad. I chose is_array because if you pass a defined variable that's not an array to array_key_exists, you get a different error thrown.

Agreed on the second point (#1). Not my bad that time ;) However, in this context it's of greater concern whether $group is empty (if it's not, we can be reasonably confident it has a nid) so this might be better:

if (!empty($group)) {
  $gid = $group->nid;
eMPee584’s picture

eMPee584 commented

what about 

$gid = isset($group->nid) ? $group->nid : NULL;

?

Anonymous’s picture

Anonymous (not verified) commented
Status: Needs review » Reviewed & tested by the community

I'll get this work committed later this week.

Anonymous’s picture

Anonymous (not verified) commented
Assigned: Unassigned »
dazweeja’s picture

dazweeja commented

In this case, it's also fine to combine the assignment and check in one (because the scope of $gid is the if block and it's beneficial to eliminate invalid values for $gid, ie. 0, '', etc.)

if ($gid = $group->nid) {
eMPee584’s picture

eMPee584 commented
NO

, is not!
What if the 'nid' property is not defined in the $group object? What if group isn't even an object? That's what's happening and that's the point at which every other programming language would simply crash. Just because PHP does not it does not mean that is valid code. E_ALL!

Anonymous’s picture

Anonymous (not verified) commented

Here's what we now have on the development branch / RC5

 $group = og_get_group_context();
 if (empty($group)) {
   ... after all is done $group is an object
 }
 if (isset($group->nid)) {
  $gid = $group->nid;
  ..
}
dazweeja’s picture

dazweeja commented

@eMPee584, "What if the 'nid' property is not defined in the $group object? What if group isn't even an object?"

There's no warning thrown with this code with E_ALL | E_STRICT. It's readable, concise, valid PHP code and the variable is correctly scoped. What if the 'nid' property is not defined? No problem, then $gid will be assigned the value NULL which is fine as there is an implicit cast to a Boolean in the if check which will fail (desired behaviour). Same goes when $group is not an object.

What is wrong with obeying all the rules of a language, in this case PHP (E_ALL | E_STRICT), and writing concise code to achieve the desired effect? It's a dynamic language - it's not Java - so these constructs are perfectly valid and often very useful. Isn't your use of isset also checking to see if a property exists on a potentially undefined object? You're happy to let PHP do the implicit check that $group exists for you before it checks that the property exists. My example takes advantage of the same implicit checking functionality. I'm sure there are languages where calling isset - or more commonly some more precise function, eg. hasProperty - to check to see if a property exists on an undefined object would throw an error or at least a warning. In fact, I'd be very surprised if you could find one of the languages that would crash in this situation that would also provide a function that would allow you to check if a property exists on an undefined object.

If the goal is correctness in all possible languages, not just PHP, shouldn't it be:

if(!empty($group) && !empty($group->nid)) {

Even this might not be enough - maybe someone might have a philosophical objection to calling empty to check to see if a property exists and yet it's still beneficial to eliminate non-useful (eg. 0, '', FALSE) values of nid, so:

if(!empty($group) && isset($group->nid) && !empty($group->nid)) {

There's all sorts of considerations with programming. Valid code is important (as my example is) but readability and conciseness is important too. Also, to be correct variables should only exist where they are useful - in this example the $gid variable should be scoped to the if block.

dazweeja’s picture

dazweeja commented

Also, when you say "that's the point at which every other programming language would simply crash", do you mean that's the point at which a static-typed programming language would crash? Because I don't agree that all other or even most dynamic languages would crash. After all, what's the point of using a dynamic-typed language if not to take advantage of it's flexibility?

eMPee584’s picture

eMPee584 commented

@dazweeja

There's no warning thrown with this code with E_ALL | E_STRICT. It's readable, concise, valid PHP code and the variable is correctly scoped. What if the 'nid' property is not defined? No problem, then $gid will be assigned the value NULL which is fine as there is an implicit cast to a Boolean in the if check which will fail (desired behaviour). Same goes when $group is not an object.

You would be right - if you were right, but alas - you're wrong. It's just that your server settings swallow the PHP notices, even though you think you set it up differently. Try putting 

error_reporting(E_ALL);
ini_set('display_errors', TRUE);
ini_set('display_startup_errors', TRUE);

right at the front of your index.php and assign undefined variables. It most certainly will throw warning messages. (If not, maybe it's your PHP version or a strange fluke in the matrix..)
Trust me (got experience XD): part of being a smartass is always adapting to realities ASAP. It's just a matter of credibility. So please try this and reflect on your words again. Oh, and that statement 'any other language would crash here' was not meant to be taken in an academically strict sense but rather as a hint how inherently wrong it is to assign undefined stuff to something.
Well and the thing about isset() - it's exactly the tool designed to do the job (checking if value is set), so obviously it will *not* choke on an undefined property. By the way - it works to check a full variable path - so if (isset($object->array['index'])) returns FALSE if either the object, it's array property or the array's index are not set, and true only if all are set (even though the value might be set to NULL).
And
if (empty($foo)) { is equivalent to (hope i got it kinda right)

 if (isset($foo) && (!is_null($foo)
  && (is_array($foo) && count($foo) > 1)
    || (is_bool($foo) && $foo === TRUE)
    || ((is_float($foo) || is_int($foo)) && $foo != 0)
    || (is_string($foo) && ($foo === '' || $foo === '0')))
    || (is_object($foo))) {

So in most situations empty() is a good and easy way to ensure input data validity. Implicit assignments will work, but are deprecated programming practice.

dazweeja’s picture

dazweeja commented

Yes, you are absolutely right and I apologise if I came off like a smart arse. There's obviously something unusual about my PHP version at work because I did change the error reporting level as you suggested and immediately saw the errors thrown by is_array (which I didn't see before) but I still couldn't get any errors triggered by #6. However, I can get those errors on my home box so there is no doubt that you are right and I'm wrong. I did take the time to code some test cases *and* I had evidence that error reporting was active but... well, something about assumptions being the mother of all...

I wasn't suggesting that isset would choke under those circumstances, just that it has a bad smell if you're used to static-typed languages. The fact that you don't have to care whether an object exists before checking for a property on that object doesn't seem to encourage good programming practices. The same can be said of empty. I actually would prefer if PHP forced me to write:

if(!empty($group) && !empty($group->nid)) {

But it doesn't so I'm personally happy using isset/empty in the way you have used it above - I just thought that you were trying to make a point about being academically strict and using isset in that way didn't really seem to be consistent with that. In fact, it turns out you were just being sensible ;)

My vote is for (so $gid has only the scope it needs):

if (!empty($group->nid)) {
  $gid = $group->nid;

Anyway, thanks for all your help. When I get error reporting working correctly at work, I'll be sure to leave it on during development. I'll go off and eat my humble pie now.

eMPee584’s picture

eMPee584 commented

well smartassing can be a positive thing (not too often but.. *g) when it helps to improve the situation. If the facts are wrong however.. smartarsing myself here .. anyways
so well guess the bug doesn't come up on your installation because you have at least one group defined. rename that og groups db table and you'll see what happens if there is no group. it's not really critical but...better E_ALL then not, huh.

dazweeja’s picture

dazweeja commented

I was trying to get the notice to show with test cases like:

if ($foo = $bar->nid) {}

I've since discovered that the reason why it wasn't showing was because these notices are swallowed by drupal_error_handler. I had to comment out line 590 and 620 in common.inc (Drupal 6.13) to get it to show from within Drupal:

//if ($errno & (E_ALL ^ E_NOTICE)) {
...
//}

This makes me wonder how you are able to see them. Is it using the Coder module? I'd rather not hack core if possible.

Anonymous’s picture

Anonymous (not verified) commented
Status: Reviewed & tested by the community » Closed (fixed)

Please reopen if this has not been resolved.

Best, Paul