drupal base and E_NOTICE [#28540]

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.

Comment	File	Size	Author
#16	patch.28540.diff	32.25 KB	beginner
#8	common.inc.enotice.patch.txt	668 bytes	Thomas Ilsche
#6	node.user.e_notice.path.txt	1.79 KB	Thomas Ilsche
#4	test.patch	25.21 KB	harry slaughter

Comments

Comment #1

killes@www.drop.org commented 9 August 2005 at 19:22

Status:

Active

» Fixed

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

Comment #2

harry slaughter

Omaha NE USA

commented 9 August 2005 at 19:32

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?

Comment #3

killes@www.drop.org commented 9 August 2005 at 19:39

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

Thanks.

Comment #4

harry slaughter

Omaha NE USA

commented 9 August 2005 at 20:24

Status	File	Size
new	test.patch	25.21 KB

attached is results of:

harry@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)

Comment #5

killes@www.drop.org commented 9 August 2005 at 21:01

Status:

Fixed

» Needs work

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.

Comment #6

Thomas Ilsche commented 11 August 2005 at 16:29

Status:

Needs work

» Needs review

Status	File	Size
new	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?)

Comment #7

Thomas Ilsche commented 11 August 2005 at 16:36

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?

Comment #8

Thomas Ilsche commented 14 August 2005 at 15:12

Status	File	Size
new	common.inc.enotice.patch.txt	668 bytes

Another patch for common inc.

Comment #9

harry slaughter

Omaha NE USA

commented 15 August 2005 at 17:30

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

Comment #10

Thomas Ilsche commented 15 August 2005 at 18:25

> 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)

Comment #11

harry slaughter

Omaha NE USA

commented 17 August 2005 at 20:19

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?

Comment #12

Thomas Ilsche commented 18 August 2005 at 04:54

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.

Comment #13

beginner commented 28 September 2005 at 09:44

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 .

cvs from 26 sept 2005 still spills dozens of errors for each page load...

Comment #14

beginner commented 13 October 2005 at 13:57

Version:	4.6.0	»
Priority:	Minor	» Normal
Status:	Needs review	» Needs work

I'm trying to polish the patch #4.

When viewing any page, there are still dozens of php error messages for each page view, coming from every include/module/file.

I need help to know what would be the way to solve this one:
notice: Undefined index: #attributes in includes/form.inc on line 53.

$form['#attributes']['class'] .= ' form-api';

How do you declare this so as to avoid the notice error??

Comment #15

beginner commented 14 October 2005 at 13:35

Ok. I traced back the last one to function system_view_general() {} in system.module

I solved the error by declaring the array at the top of the function, adding $form['#attributes']['class'] = array ();:

function system_view_general() {

  $form['#attributes']['class'] = array ();

  // General settings:
  $form['general'] = array(
    '#type' => 'fieldset', '#title' => t('General settings'),
    '#collapsible' => TRUE, '#collapsed' => TRUE
  );

I'm not sure that's the right way to do it, though. Let me know if it isn't.