[drupal-devel] [bug] drupal base and E_NOTICE
hlslaughter
drupal-devel at drupal.org
Wed Aug 17 20:19:43 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: hlslaughter
Status: patch (code needs review)
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?
hlslaughter
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)
More information about the drupal-devel
mailing list