E_ALL Compliance: Fix PHP Notices in issue.inc [#103795]

issue.inc produces various PHP Notices with E_ALL on. These are all due to variables, array keys or object fields which may not exist when used.

This patch fixes those that I spotted, by changing to equivalent behaviour without Notices.

Examples:

  switch ($_POST['op'] ? $_POST['op'] : arg(2)) {

changed to

  switch (!empty($_POST['op']) ? $_POST['op'] : arg(2)) {

        '#default_value' => $status[$nid],

changed to

        '#default_value' => isset($status[$nid]) ? $status[$nid] : NULL,

  $pid = $node->pid ? $node->pid : $_POST['edit']['pid'];

changed to

  $pid = !empty($node->pid) ? $node->pid : (isset($_POST['edit']['pid']) ? $_POST['edit']['pid'] : NULL);

    if (trim($project->help)) {

changed to

    if (isset($project->help) && trim($project->help)) {

    '#default_value' => ($query->states) ? $query->states : array(1, 2, 8, 13, 14),

changed to

    '#default_value' => !empty($query->states) ? $query->states : array(1, 2, 8, 13, 14),

Best wishes,

Mark.

Comment	File	Size	Author
#2	project_issue_103795.patch	5.66 KB	drewish
	issue.inc_3.patch	14.33 KB	plumbley

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

dww

we/he/they

CreditAttribution: dww commented 13 July 2007 at 04:38

Status:

Needs review

» Needs work

a) Doesn't apply cleanly anymore (my fault, sorry it took so long to get to this)

b) this:

+  if (isset($releases) && count($releases)) {

would be better as:

+  if (!empty($releases)) {

c) this:

+  $output .= isset($group) ? $group : NULL;

would be better as:

+  if (isset($group)) {
+     $output .= $group;
+  }

d) same here:

-  $output .= "$rss $link";
+  $output .= (isset($rss) ? $rss : '') . ' ' .(isset($link) ? $link : '');

your version is hard to understand, and doesn't follow our coding conventions regarding spaces and string concatenation.

otherwise, this is a good start, thanks! we'll certainly want something like this before we start porting to D6, so we might as well get this fixed up and committed to all branches.

also, even though i already committed the other patch that included a bunch of similar code, I'm having second thoughts about this style:

+      '#default_value' => isset($query->priorities) ? $query->priorities : NULL

I wonder if this would be better for both readability and speed (after the initial array definition):

  if (isset($query->priorities)) {
     $form['priorities']['#default_value'] = $query->priorities;
  }

I'm not sure which I prefer, but I wanted to raise it in case anyone else sees this issue and has strong opinions. ;)

Comment #2

drewish CreditAttribution: drewish commented 20 September 2007 at 16:39

Version:	4.7.x-1.0	» 5.x-2.x-dev
Status:	Needs work	» Needs review

File	Size
project_issue_103795.patch	5.66 KB

tried to apply these by hand to HEAD and then worked on cleaning up some others.

Comment #3

hunmonk CreditAttribution: hunmonk commented 27 September 2007 at 17:24

Status:

Needs review

» Fixed

applied with some minor adjustments to 5.x-1.x. doesn't apply at all to either 4.7 branch -- i'll leave it as to be ported if anybody is interested,,,