Lots of cheeses standing alone.

CommentFileSizeAuthor
#3 drupal-undefined-variables.2.patch25.01 KBcarlos8f
drupal-undefined-variables.patch25.56 KBcarlos8f

Comments

carlos8f’s picture

carlos8f commented

This is partially inspired by #684138: DatabaseConnection_sqlite->dbNextId() always returns NULL, breaking batch api, installation, where the SQLite db_next_id() implementation was completely broke and no one noticed. The implementation returned an undefined variable, and it was obvious that someone had just copied and pasted the code and not tested it.

Undefined variables are easy to spot with software, any decent IDE should point them out. In some cases they are harmless and in others fatal, so I think it's not something to ignore. It also suggests that our test coverage is lacking in these spots.

Status: Needs review » Needs work

The last submitted patch, drupal-undefined-variables.patch, failed testing.

carlos8f’s picture

carlos8f commented
Status: Needs work » Needs review
StatusFileSize
new drupal-undefined-variables.2.patch25.01 KB

Note that I'm pre-defining variables in some cases where it isn't really necessary, but this is to assist in auto-detecting accidentally undefined variables. Most software isn't smart enough to check included files or compute an extract() call.

dries’s picture

dries commented

Interesting. Lots of things to clean-up it seems. By the way, we tend to write @todo instead of just TODO.

casey’s picture

casey commented
Status: Needs review » Needs work

Needs a reroll.

Blooniverse’s picture

Blooniverse commented

Maybe my little story is of interest for you?:

In order to get some metatags into Drupal7 (no nodewords, no meta tag module available yet) I typed the following code into Bartik's page.tpl.php, just on top of everything else:

 $element = array(
 '#tag' => 'meta', 
 '#attributes' => array( 
  'name' => 'description',
  'content' => $node->title.' — '.$node->type.' — '.substr(strip_tags(trim($node->body['de'][0]['safe_value'])), 0, 100)
  )
 );
 drupal_add_html_head($element, 'ps_header_meta');
// $Id: page.tpl.php,v 1.9 2010/11/07 21:48:56 dries Exp $

The result is, every now and then (means: sometimes things are okay, sometimes not):

Notice: Undefined variable: node in include() (Zeile 6 von /var/www/site.net/themes/bartik/templates/page.tpl.php).
Notice: Trying to get property of non-object in include() (Zeile 6 von /var/www/site.net/themes/bartik/templates/page.tpl.php).
Notice: Undefined variable: node in include() (Zeile 6 von /var/www/site.net/themes/bartik/templates/page.tpl.php).
Notice: Trying to get property of non-object in include() (Zeile 6 von /var/www/site.net/themes/bartik/templates/page.tpl.php).
Notice: Undefined variable: node in include() (Zeile 6 von /var/www/site.net/themes/bartik/templates/page.tpl.php).
Notice: Trying to get property of non-object in include() (Zeile 6 von /var/www/site.net/themes/bartik/templates/page.tpl.php).
pasqualle’s picture

pasqualle
Location 🇭🇺 Budapest
commented
Status: Needs work » Needs review
Issue tags: -code cleanup, -Cleanup, -API clean-up, -D7 API clean-up, -clean-up

drupal-undefined-variables.patch queued for re-testing.

poker10’s picture

poker10 commented
Issue summary: View changes
Status: Needs review » Needs work

Thanks for working on this. This still Needs work, as there are open @todos. Also the patch does not apply anymore.

Status: Needs work » Closed (outdated)

Automatically closed because Drupal 7 security and bugfix support has ended as of 5 January 2025. If the issue verifiably applies to later versions, please reopen with details and update the version.