Error_reporting, oh well...

By Lucideye on 10 Jun 2005 at 12:58 UTC

I am currently writing a module and couldn't figure out why a certain thing didn't work. After half an hour of searching, I discovered that a variable was wrongly spelled. I thought: "What's that? I always have error_reporting set to E_ALL." So I looked how I could set it to E_ALL in Drupal. I tried different things until I discovered the following line in the error_handler() function in common.inc:

if ($errno & (E_ALL ^ E_NOTICE)) {

I changed it to:

if ($errno & (E_ALL)) {

I was a bit shocked as I saw that suddenly DOZENS of php notices were getting displayed along with my desired: "Undefined variable" notice that I was needing for debugging purposes.

Well, as great as Drupal is in a way, but in my opinion this a SERIOUS flaw. Believe me, it is possible to write code that is free of PHP notices, and if you need error_reporting(E_ALL) for debugging purposes, you can't use it, because of Drupal's error_handler() that filters everything out.

Drupal devs, you can do better than that, don't you think??

Comments

because its interesting i

micha_1977 commented 10 June 2005 at 13:34

because its interesting i changed my common.inc the same way

but

it seems that for example
notice: Undefined variable: attributes in \themes\cake\template.php on line 25.

comes from

          unset($attributes);
          if (!empty($value['text'][$i])) {
            if (!empty($value['description'][$i])) {
              $attributes['title'] = $value['description'][$i];
            }
            $text = $value['text'][$i];
            $link = $value['link'][$i];
/* the original from phptemplate.engine changed to catch the produced links
*/
            if (substr($link, 0, 4) == 'http') {
              $_templink = '<a href="/'. $link .'"'. drupal_attributes($attributes) .'>'. $text .'</a>';
            }
            else {
              $_templink = l($text, $link, $attributes);
            }
/* reproduced "l" function from common.inc to get the active link, only for "primary links" 
   classes are set acccording to my .style.css, $classtext is needed for additional css logik
*/

the phpcode looks just fine to me, so..its a notice not a warning ;-)

-micha
work in progress with Drupal 4.6: langmi.de

bug report

sepeck commented 10 June 2005 at 14:31

What's up with this? "Drupal devs, you can do better than that, don't you think??" Little snide asides are not necessary.

IF you think you've found a bug, go here http://drupal.org/project/issues/3060 and search to see if someone else found it. If it's not there, then post one. Check back to see if there are comments on it. If you are really concerned, subscribe to the drupal devel list and chat about it.

I will point out that accomadating error handling is an important part of coding.

-sp
---------
Test site...always start with a test site.
Drupal Best Practices Guide

-Steven Peck
---------
Test site, always start with a test site.
_{Drupal Best Practices Guide}

You are right. I may have

Lucideye commented 10 June 2005 at 16:09

You are right. I may have overreacted a bit, maybe because I am converting from Xaraya where already the slightest PHP notice raises a system exception, and actually I take it for granted that a well coded aplication doesn't raise any notices. But, nonetheless, I still think that this is a serious flaw, and I do not mean the user defined error_handling, but the grubby coding practice, like e.g.

if ($array['key']) {
  return $array;
}

which has to be:

if (isset($array['key'])) {
  return $array;
}

Another practice that I think is suboptimal is the ample use of nested statements and conditions. It is also considered bad practice to write code like:

function foo()
{
  if ($condition1) {
    if ($condtion2) {
      if ($condition3) {
        return 'something';
     }
   }
  }
}

contrary to:

function foo()
{
  if (!$condition1) return false;
  if (!$condition2) return false;
  if (!$condition3) return false;
  return 'something';
}

I think that maybe some reasons because there are many devs who refer to Drupal as just being "weird".

And at first I thought so, too. But when I digged deeper, I found that the hook system is great and of course there are many more great things about it, and I think, for developers, it is really one of the best things since sliced bread, because it is so damn easy to write your own modules, so in this regard it is really great (contrary to Xaraya with its one function per file, separation of user/admin and regular/api functions and many more puritanical directives), but however I think it could be a bit more clean.

Sorry, if I have offended anyone and thanks for Drupal which is a great system with huge potential and I believe it will constantly become better.

Thanks again,
Alex

heh

sepeck commented 10 June 2005 at 17:05

Not offended, just trying to help you get the best response on your concerns. As an fyi, anyone can submit a patch. If you do not think that something is done 'correctly', then you can make a patch and submit it against Drupal core. Write why you think it needs to be applied, any testing you've done, etc and attach your diff to the issue you create.

Participation on the dev list will generate more attention. Response to any feedback you get on your patch is even better. Anyone can submit a patch, and as long as it passes review it may get included. Drupal truely is open. Now, only 3 people can commit core patches and there are the various area 'leads' that have developed over time so happy coding and welcome to Drupal.

-sp
---------
Test site...always start with a test site.
Drupal Best Practices Guide

-Steven Peck
---------
Test site, always start with a test site.
_{Drupal Best Practices Guide}

same concern

harry slaughter

Omaha NE USA

commented 9 August 2005 at 17:25

the thing that really bugs me is that because drupal produces so many notice level errors, it's not very practical to turn on E_ALL. so i leave it off, and in the process miss notice messages that i would normally use as reminders to clean up my stuff. so i'm producing the same lazy code :(

i think the underlying problem is that drupal doesn't seem to have any formalized testing standards. some would probably consider it unnecessary/extreme, but requiring code that produces no notices wouldn't hurt drupal and would most likely help.

IMHO, code that produces "harmless" warnings is just a result of lazy (or worse, sloppy) programming. i totally appreciate that in a dev environment. my scripts in progress generally produce more noise than jesse jackson. most of my experience is with Perl, and I learned early on the benefits of "use strict", which prevents sloppy scripts from even compiling. initially it was a nuisance to do stuff like initializing vars when i "knew what i was doing". but learning to always do this eventually became a habit, and who knows how many bugs i've avoided for doing things this way.

i think drupal would do well to adopt some minimal rules for the code that goes into non-dev releases.

unfortunately, there doesn't seem to be a lot of interest in this. i've volunteered to help in more formal testing, but i've just been told to submit patches instead. well, i'm not interested in submitting work to a module queue that hasn't been looked at in months :) i'd rather help raise the bar for what's considered "stable".

anyway....

--
Devbee - http://devbee.net/

Thank you hslaughter for

beginner commented 28 September 2005 at 09:41

Thank you hslaughter for your help on IRC.
My case is another practical example why having E_ALL Notices on and clean all the small bugs is important.
see #6 http://drupal.org/node/32262 .

--
http://www.reuniting.info/
Healing with Sexual Relationships.

A per-module fix for E_NOTICE

robb commented 22 February 2007 at 03:26

This just bit me today. My solution is slightly different as it allows me to turn on/off E_NOTICE per module. This helps limit the debug output to just the stuff I am responsible for.

Like everyone else I modified includes/common.inc

function error_handler($errno, $message, $filename, $line) {
  // If the @ error suppression operator was used, error_reporting is temporarily set to 0
  if (error_reporting() == 0) {
    return;
  }

  // New code to selectively turn on E_NOTICE for some modules
  $see_err = FALSE;
  if (($errno & E_NOTICE) && is_array($GLOBALS['SEE_E_ALL'])) {
    if(in_array($filename, $GLOBALS['SEE_E_ALL'])) {
      $see_err = TRUE;
    }
  }

  if (($errno & (E_ALL ^ E_NOTICE)) || $see_err) {

// Back to the original code

In modules I want to enable E_NOTICE in I just add the following to the top of my module/theme/whatever file.

$GLOBALS['SEE_E_ALL'][] = __FILE__;

I'm sure there is a better solution but in the 10 minutes I had this works fine.

Error_reporting, oh well...

Comments

because its interesting i

bug report

You are right. I may have

heh

same concern

Thank you hslaughter for

A per-module fix for E_NOTICE

New forum topics

News items

Our community

Documentation

Drupal code base

Governance of community