E_ALL Compliance: Fix all remaining notices

Merged in some I did yesterday

I've committed this patch. I'm marking this 'active' rather than 'fixed' as I'm sure there is more. ;-)

http://cvs.drupal.org/viewcvs/drupal/drupal/modules/user/user.module?r1=... I am very sure that something will break because of this change -- instead of moving specifically named form elements you do everything. This kind of change is not covered by the (unwritten) E_ALL rules...

As I read further, more and more badness:

      if ($_POST['accounts'] && $_POST['operation'] == 'delete') {	       if (isset($_POST['accounts']) && $_POST['operation'] == 'delete') {

this is not the same what if post accounts is set but empty? You are changing functionality. Do not do that. Sigh, now I need to read all files changed to pinpoint errors.

menu.inc: if ($link['link']) { if (isset($link['link'])) {

module.inc of course you can write tons of code, but the short answer is this:

    $files[$filename]->info = $file->info + array('version' => NULL);

theme.inc $image_attributes = isset($image_attributes) ? $image_attributes : ''; in module.inc you used if but here, where it would be correct you did not.

block.module

   $row[] = $block['delete'] ? drupal_render($block['delete']) : '';	       $row[] = isset($block['delete']) ? drupal_render($block['delete']) : '';

again not the same... but this one may pass.

and the last chunk in block, again:

  return $edit + array('block' => array());

form.inc has this construct since eternity for element.info and menu.inc uses it to load default values, too.

book needs to use !empty not isset... this is true for some places I mentioned already:

empty() is the opposite of (boolean) var, except that no warning is generated when the variable is not set.

node

  if ($_POST['operation'] == 'delete' && $_POST['nodes']) {	   if (isset($_POST['operation']) && $_POST['operation'] == 'delete' && $_POST['nodes']) {

you missed the isset post nodes.

system.module $block += array('content' => '');, if (isset($block['block callback']) && function_exists($block['block callback'])) { again you want !empty not isset same with $style->prefix = $theme->template ? $theme->prefix : $theme->name; $style->prefix = isset($theme->template) ? $theme->prefix : $theme->name; -- this is so broken that I raised this to critical.

   return (isset($a['weight']) || isset($b['weight'])) ? $a['weight'] - $b['weight'] : strcmp($a['title'], $b['title']);
   return (isset($a['weight']) && isset($b['weight'])) ? $a['weight'] - $b['weight'] : strcmp($a['title'], $b['title']);

you got to be kidding!

if (!$block['position']) { if (!isset($block['position'])) { that's empty

taxonomy
if (isset($form['taxonomy']) && is_array($form['taxonomy']) && !empty($form['taxonomy'])) { lose isset, move !empty to the forefront.

and while at it, please change the occasionnal sizeof to count, they are alias but core more uses count.

http://www.blueshoes.org/en/developer/php_cheat_sheet

This has been rolled back.

To sum up, some general tips:

• If you see if ($foo) that's a cast to boolean and checking for TRUE. As I already quoted,
  

  if (!empty($foo)) is the exact same just it does not give you a notice and there is no speed penalty. Both stem from the fact that empty is not a PHP function, it's a language construct.</li>
  <li>Defaults are better than checking later. The + array operator is great to add defaults to arrays, as <code>$a + $b

  $b values does not overwrite same key $a values. There is a += operator too, and we like it.

• Changing behavior in an E_ALL patch is unacceptable.

To sum up, some general tips:

• If you see if ($foo) that's a cast to boolean and checking for TRUE. As I already quoted, if (!empty($foo)) is the exact same just it does not give you a notice and there is no speed penalty. Both stem from the fact that empty is not a PHP function, it's a language construct.
• Defaults are better than checking later. The + array operator is great to add defaults to arrays, as $a + $b $b values does not overwrite same key $a values. There is a += operator too, and we like it.
• Changing behavior in an E_ALL patch is unacceptable.

Small patch to fix this and that. I do not believe in catch all patches. Small is beautiful. As I go and code menu stuff , I catch notices and fix them. All shall do similarly instead of hunting them specifically IMO. This way the patch is better tested.

borken patch

More!

This time we caught a very minor bug in menu.inc. E_ALL hunting is good.

Removed settings.php chunk

There was still a ", tab" in the menu_rebuild() SQL (left from chx's menu tabs branch)

+    db_query("INSERT INTO {menu} (mid, pid, path, access_callback, access_arguments, page_callback, page_arguments, map_callback, map_arguments, fit, number_parts, vancode, menu_link, visible, parents, depth, has_children, tab) VALUES (%d, %d, '%s', '%s', '%s', '%s', '%s', '%s', '%s', %d, %d, '%s', '%s', %d, '%s', %d, %d)", $insert_item['_mid'], $insert_item['_pid'], $path, $insert_item['access callback'], serialize($insert_item['access arguments']), $insert_item['page callback'], serialize($insert_item['page arguments']), $insert_item['map callback'], serialize($insert_item['map arguments']), $insert_item['_fit'], $insert_item['_number_parts'], $vancode .'+', $link, $insert_item['_visible'], $insert_item['_parents'], $insert_item['_depth'], $insert_item['_has_children']);

We love notices.

More, more and more!

webchick found even more.

One more notice caught and a logic flaw is now correct... in element_child we have $key[0] != '#';, now if $key[0] is not set, then this is certainly true.

I think a menu_rebuild(); is required after the content types page and story are added dynamically via the install profile _profile_final().

Adding a menu_rebuild() after the _profile_final() call in install.php which works but it may not be the ideal location (but I suppose you could do anything that requires a menu rebuild in _profile_final() not just adding content types).

You are right but that's a different issue:http://drupal.org/node/113788

I can't repro that bug. I will those in the pastebin now

I'd like to help getting this patch into core.

I think it is hard to get a 70k patch into core. Can we split it up a bit? I suggest splitting it up per directory:

1) modules
2) includes
3) themes
4) Root.

The attached patch fixes some more notices I found.

Committed to CVS. Thanks.

killes: I missed your patch. I was looking at the previous patch when you submitted yours. Can you updated your tree and reroll? Thanks.

I've found plenty more notices.

Method:

- log in as user #1
- uncomment the "logout" code in user.module
- call menu_rebuild();
- truncate watchdog;
- wget -r --load-cookies="/path/to/cookies.txt" http://localhost/

Afterwards you'll have your watchdg log full of notice warnings.

Updated patch attached.

chx has spotted some errors, but I can't work on it now.

I see a ton of notices when enabling the poll module- should I make a separate issue for that module? I don't see it in the last patch.

Ok, starting with the patch in #36:

Hunk #1 FAILED at 1038- looks like it's already committed.

note- in this patch I added this code:

  if (!isset($node->title)) {
    $node->title = '';
  }
  if (!isset($node->body)) {
    $node->body = '';
  }

to the end of node_object_prepare(), which catches notices about unset title and body in poll, book, and maybe other modules.

Poll module is a disaster- some of it's not even updated properly for 5.x, so I couldn't fix all the notices there. It also brought up an interesting new set of notices in node module, since the poll type doesn't have a body or format.

I should have marked this as "needs review" to get some attention. I'll double check that it still applies cleanly.

There's a also now an annoying warning from the menu module after the conversion to the brilliant new menu system:

warning: Invalid argument supplied for foreach() in /Users/Shared/www/drupal6/includes/menu.inc on line 253.

Attached patch re-rolled and fixes an additional undefined index (left, right) in system module, and leaves out one line in system module that's already been fixed elsewhere. Otherwise, the patch is the same just with different line numbers.

Almost all these changes are trivial, but please review!

do not switch on menu module yet.

@chx- sorry, but I don't know what you mean. The item above relating to menu.inc is not something I tried to address in the patch (the patch contains no changes in menu.inc), it was just an observation. Sorry for clouding the issue.

Regarding the profile module's notices:
1/ I think we can add a new one in profile_view_field, it should be
if (isset($user->{$field->name}) && $value = $user->{$field->name}) { instead of if ($value = $user->{$field->name}) { when browsing an user profile without any datas.
I would like to roll this patch again but it's very broken for the moment (I guess for chx menu's change, so I'll wait a little.

2/ This patch update all $form['fields'] blindly in profile_field_form.
It leads to some problems.
Ex: we have

  if ($type == 'selection') {
    $form['fields']['options'] = array('#type' => 'textarea',
      '#title' => t('Selection options'),
      '#default_value' => isset($edit['options']) ? $edit['options'] : '',
      '#description' => t('A list of all options. Put each option on a separate line. Example options are "red", "blue", "green", etc.'),
    );
  }

which is only available while creating a "selection" profile field. So when doing the INSERT (in profile_field_form_submit), $form_values['options'] isn't set so lead to a NOTICE.

So I think we should better do the check for isset at query level db_query("INSERT INTO... ", %s ..., $form_values['title'], ..., isset($edit['options']) ? $edit['options'] : '', ...);

I don't think this one is going to fly. Closing outright.

Autopatch Results for notice_followup_2.patch:
patching file modules/color/color.module
Hunk #1 succeeded at 157 with fuzz 2 (offset 10 lines).
patching file modules/forum/forum.module
Hunk #1 succeeded at 472 (offset 29 lines).
Hunk #3 succeeded at 498 (offset 29 lines).
Hunk #4 FAILED at 510.
Hunk #5 succeeded at 508 with fuzz 1 (offset 6 lines).
1 out of 5 hunks FAILED -- saving rejects to file modules/forum/forum.module.rej
patching file modules/node/content_types.inc
Hunk #1 FAILED at 34.
Hunk #2 succeeded at 141 (offset -4 lines).
1 out of 2 hunks FAILED -- saving rejects to file modules/node/content_types.inc.rej
patching file modules/node/node.module
Hunk #1 succeeded at 355 with fuzz 1 (offset 63 lines).
Hunk #2 FAILED at 642.
Hunk #3 FAILED at 737.
Hunk #4 FAILED at 1590.
Hunk #5 succeeded at 2043 with fuzz 1 (offset 121 lines).
Hunk #6 succeeded at 3025 (offset 89 lines).
Hunk #7 FAILED at 3036.
misordered hunks! output would be garbled
Hunk #8 FAILED at 3006.
5 out of 8 hunks FAILED -- saving rejects to file modules/node/node.module.rej
patching file modules/poll/poll.module
Hunk #1 succeeded at 133 (offset 21 lines).
Hunk #2 FAILED at 199.
Hunk #3 succeeded at 225 (offset -4 lines).
Hunk #4 succeeded at 412 (offset 21 lines).
Hunk #5 FAILED at 430.
Hunk #6 succeeded at 435 (offset -2 lines).
Hunk #7 succeeded at 541 (offset 21 lines).
Hunk #8 FAILED at 633.
3 out of 8 hunks FAILED -- saving rejects to file modules/poll/poll.module.rej
patching file modules/profile/profile.module
Hunk #1 succeeded at 255 (offset 25 lines).
Hunk #3 succeeded at 299 (offset 25 lines).
patching file modules/system/system.install
Hunk #1 succeeded at 87 (offset -5 lines).
patching file modules/system/system.module
Hunk #1 FAILED at 1498.
Hunk #2 FAILED at 1853.
Hunk #3 succeeded at 2507 (offset 316 lines).
2 out of 3 hunks FAILED -- saving rejects to file modules/system/system.module.rej
patching file modules/upload/upload.module
Hunk #1 FAILED at 344.
1 out of 1 hunk FAILED -- saving rejects to file modules/upload/upload.module.rej
patching file modules/user/user.module
Hunk #1 FAILED at 1835.
Hunk #2 succeeded at 2940 (offset 279 lines).
Hunk #3 FAILED at 2979.
2 out of 3 hunks FAILED -- saving rejects to file modules/user/user.module.rej
can't find file to patch at input line 532
Perhaps you used the wrong -p or --strip option?
The text leading up to this was:
--------------------------
|Index: modules/watchdog/watchdog.module
|===================================================================
|RCS file: /cvs/drupal/drupal/modules/watchdog/watchdog.module,v
|retrieving revision 1.171
|diff -u -p -r1.171 watchdog.module
|--- modules/watchdog/watchdog.module 27 Feb 2007 12:29:22 -0000 1.171
|+++ modules/watchdog/watchdog.module 18 Mar 2007 01:58:47 -0000
--------------------------
File to patch:
Skip this patch? [y]
Skipping patch.
1 out of 1 hunk ignored
patching file includes/install.inc
patching file includes/theme.inc
Reversed (or previously applied) patch detected! Assume -R? [n]
Apply anyway? [n]
Skipping patch.
1 out of 1 hunk ignored -- saving rejects to file includes/theme.inc.rej