[drupal-devel] [bug] drupal base and E_NOTICE

Thu Aug 18 04:54:30 UTC 2005

Issue status update for 
http://drupal.org/node/28540
Post a follow up: 
http://drupal.org/project/comments/add/28540

 Project:      Drupal
 Version:      4.6.0
 Component:    base system
 Category:     bug reports
 Priority:     minor
 Assigned to:  Anonymous
 Reported by:  hlslaughter
 Updated by:   Thomas Ilsche
 Status:       patch (code needs review)

This is my personal opinion,
I do not like any @ usage. Those errors are there for a reason. They
help finding real bugs. For E_NOTICE epsecially for typos, imagine
those index is not just missing, its simply mistyped but hardly used...

I prefer to initialize variables as much as possible. If it really is
necesary isset.
The problem with if($var['index']) is that it can have different
meanings:
Check if the index exist (bogus for 0, ''...)
Check if $var['index'] is actually 0 / '' / NULL
Use a real boolean value,
Any combination of this.

when someone reads this concise form it is hardly possible to tell what
meaning it should have. This is why I personally prefer more detailled
checks using isset,==(=),is_null etc.

And as mentioned already they do help finding typos/bugs. If
$var['index'] is supposed to exist and you get a notice then you know
there is something wrong.

Thomas Ilsche

Previous comments:
------------------------------------------------------------------------

Tue, 09 Aug 2005 19:19:33 +0000 : hlslaughter

ok, instead of whining in the forums, i'll submit a ticket.

i'm sure i'm not the first person to bring this up, and i apologize in
advance if this issue is already decided/closed.

but my issue is this: would it be possible to review the core modules
and clean up code to the point that it would be feasible to run drupal
with notices turned on (at least in a dev environment)?

i think it would be pretty trivial to clean most/all of these errors
from the code base. the cost of this effort would be relatively teeny,
and the benefits would be great. admins would no longer panic if they
happen to see these messages; with an option added to the settings
screen, developers could turn on notices and would write cleaner code,
which would probably result in fewer bugs in 3rd party modules; etc...
you know the routine 8*)

after turning on E_NOTICE in common.inc, i see the following notice
messages when accessing the homepage of my site:

occurences - message
----V--------------V--------
     12 notice: Undefined index:  field in
/var/www/html/includes/tablesort.inc on line 114.
     10 notice: Undefined variable: teaser in
/var/www/html/modules/opt/attachment.module on line 37.
      6 notice: Undefined variable: count in
/var/www/html/modules/opt/attachment.module on line 120.
      6 notice: Undefined index:  field in
/var/www/html/includes/tablesort.inc on line 176.
      4 notice: Undefined variable: rows in
/var/www/html/modules/opt/attachment.module on line 352.
      4 notice: Undefined variable: count in
/var/www/html/modules/opt/attachment.module on line 123.
      4 notice: Undefined variable: attachments in
/var/www/html/modules/opt/attachment.module on line 279.
      1 notice: Undefined variable: node in
/var/www/html/modules/node.module on line 1950.
      1 notice: Undefined offset:  0 in
/var/www/html/includes/pager.inc on line 59.
      1 notice: Undefined index:  op in
/var/www/html/modules/node.module on line 1636.
      1 notice: Undefined index:  node.taxonomy in
/var/www/html/themes/engines/xtemplate/xtemplate.inc on line 164.
      1 notice: Undefined index:  node.picture in
/var/www/html/themes/engines/xtemplate/xtemplate.inc on line 164.
      1 notice: Undefined index:  header.title in
/var/www/html/themes/engines/xtemplate/xtemplate.inc on line 164.
      1 notice: Undefined index:  header.site_slogan in
/var/www/html/themes/engines/xtemplate/xtemplate.inc on line 164.
      1 notice: Undefined index:  header.mission in
/var/www/html/themes/engines/xtemplate/xtemplate.inc on line 164.
      1 notice: Undefined index:  header.help in
/var/www/html/themes/engines/xtemplate/xtemplate.inc on line 164.
      1 notice: Undefined index:  from in
/var/www/html/includes/pager.inc on line 53.
      1 notice: Undefined index:  footer.message in
/var/www/html/themes/engines/xtemplate/xtemplate.inc on line 164.
      1 notice: Undefined index:  footer.blocks in
/var/www/html/themes/engines/xtemplate/xtemplate.inc on line 164.
      1 notice: Undefined index:  edit in
/var/www/html/modules/node.module on line 1637.
      1 notice: Undefined index:  2 in
/var/www/html/modules/filter.module on line 629.
      1 notice: Undefined index:   in
/var/www/html/includes/common.inc on line 506.
      1 notice: Trying to get property of non-object in
/var/www/html/modules/node.module on line 1950.

again, no offense, and sorry if i'm trying to bring a dead horse back
to life.

------------------------------------------------------------------------

Tue, 09 Aug 2005 19:22:44 +0000 : killes at www.drop.org

This is being worked on for the next Drupal version. Please install
yourself a test site on cvs and submit individual patches.

------------------------------------------------------------------------

Tue, 09 Aug 2005 19:32:57 +0000 : hlslaughter

thanks, i'll do this.

i take it patches are just posted here? do i just post a diff -N, title
it patch and mark the component appropriately?

------------------------------------------------------------------------

Tue, 09 Aug 2005 19:39:08 +0000 : killes at www.drop.org

Yes, attach here or create new issue.
diff -up is what we usually use. 

Thanks.

------------------------------------------------------------------------

Tue, 09 Aug 2005 20:24:16 +0000 : hlslaughter

Attachment: http://drupal.org/files/issues/test.patch (25.21 KB)

attached is results of:

harry at penny:~/cvs/third_party/drupal/drupal> diff -Naurp .
/var/www/html/drupal_cvs/ > ../test.patch

ok, would this type of patch be suitable? 

it includes fixes for all the E_NOTICE messages generated when
accessing the homepage of a new, unmodified drupal installation
straight from CVS (as of Aug  9 13:14)

------------------------------------------------------------------------

Tue, 09 Aug 2005 21:01:37 +0000 : killes at www.drop.org

You are almost there. 

How to best prepare patches is to run Drupal directly from a cvs
checkout. then you can fix the checkout and then run cvs diff -up 

This way the unrelated files in the patch won't be included.

------------------------------------------------------------------------

Thu, 11 Aug 2005 16:29:12 +0000 : Thomas Ilsche

Attachment: http://drupal.org/files/issues/node.user.e_notice.path.txt (1.79 KB)

I have a small patch for node.module and user.module that fixes 3
notices in the current HEAD.
Please especially have a look at node_access_view_all_nodes, I simply
do not get the current use of $node->nid
(do I need some more sleep or is this really bogus?)

------------------------------------------------------------------------

Thu, 11 Aug 2005 16:36:22 +0000 : Thomas Ilsche

About the newlines at eof:
Sorry didn't notcied this as phped removed them manually, is there any
reason for them, I got them in many drupal files? Maybe worth a fix
aswell?

------------------------------------------------------------------------

Sun, 14 Aug 2005 15:12:00 +0000 : Thomas Ilsche

Attachment: http://drupal.org/files/issues/common.inc.enotice.patch.txt (668 bytes)

Another patch for common inc.

------------------------------------------------------------------------

Mon, 15 Aug 2005 17:30:34 +0000 : hlslaughter

just curious, any special reason you're using "!empty()" instead of
"isset()"? i'd think that an empty string is technically a valid value
(though the end result is the same codewise). 

also, sorry i haven't submitted patches, i guess i'm not confident i
understand the correct method to submit. is it as follows?

1) cd $drupal_cvs/somedir/
(optionally: cvs update .)
2) edit some.file
3) cvs diff -up > some.file.patch
4) attach some.file.patch to appropriate thread at drupal.org
5) assumming patch is good and correct, pray that someone notices the
patch and applies it within CVS

?

------------------------------------------------------------------------

Mon, 15 Aug 2005 18:25:15 +0000 : Thomas Ilsche

> just curious, any special reason you're using "!empty()" instead of
"isset()"? i'd think that an empty string is technically a valid value
(though the end result is the same codewise).

I wanted to keep it semantically equivalent to if($var). isset is not.
This might be especially critical against boolean values.
The very problem is the fact that many properties of objects / indexes
of arrays are not initialized. !empty is simply on the safe side.

I think with the patches it is important to do them from the main
drupal dir and not subdirs (but I'm not sure either, a
faq/tut/introduction would be cool)

------------------------------------------------------------------------

Wed, 17 Aug 2005 20:19:36 +0000 : hlslaughter

i noticed that some folks are silencing errors instead of doing
long-hand

1) not ideal makes "E_ALL" impractical:
$foo = $baz["val"]; // generates notice if "val" is not set
2) cleaner:
if ( isset($baz["val"]) ) {
  $foo = $baz["val"];
}
// or my preference:
$foo = isset($baz["val"]) ? $baz["val"] : NULL;
3) alt way:
$foo = @$baz["val"];

methods 1 and 2 can become very ugly and hard to read if you're dealing
with long/nested arrays. 

i like method 3 as it's easy to read and the silencer/@ indicates that
the author is aware this syntax may generate a warning notice, but it's
ok. 

what do you guys think?