project.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:
1.

  if ($_GET['edit'][$field]) {

changed to

  if (!empty($_GET['edit'][$field])) {

2.

  '#default_value' => $node->title,

change to 

  '#default_value' => isset($node->title) ? $node->title : NULL,

3. Additional

  $output = '';

in project_project_view() which uses $output .= ... later.

4.

  return $node;

changed to

  return isset($node) ? $node : NULL;

since $node may not be set at point of return

Best wishes,

Mark.

CommentFileSizeAuthor
#3 project_103790.patch5.56 KBdrewish
project.inc_2.patch6.63 KBplumbley

Comments

plumbley’s picture

plumbley commented
Status: Active » Needs review

(Setting correct status)

drewish’s picture

drewish commented
Version: 4.7.x-1.0 » 5.x-1.x-dev

re-rolled for 5. made some small changes. in project_project_view() it seemed easier to just return once we had something and return null if we hadn't already returned.

i did a little testing on this and everything looked correct.

drewish’s picture

drewish commented
StatusFileSize
new project_103790.patch5.56 KB

here's that patch

hunmonk’s picture

hunmonk commented
Status: Needs review » Patch (to be ported)

applied to HEAD. patch doesn't apply cleanly to 4.7's

also, note that:

-  $form['project']['format'] = filter_form($node->format); 
+  $form['project']['format'] = filter_form(isset($node->format) ? $node->format : NULL);

is wrong. i think FILTER_FORMAT_DEFAULT needs to be used instead of NULL.

plumbley’s picture

plumbley commented

@hunmonk - I agree it is probably not the intended use of $format, but I have deliberately not changed any behaviour here: the previous behaviour was that if $node->format was not explicitely set (which can happen in simple custom modules without a format field), the format will de-facto be set to NULL, with a PHP notice if enabled. Now we make it explicit, it is clear this is not the desired behaviour.

But since having stray NULL format seems a fairly common pattern for other modules, it may be safer to catch this specifically in e.g. filter_resolve_format() in filter.module (at least in 4.7), i.e. that filter specifically defaults a NULL format to FILTER_DEFAULT_FORMAT?

hunmonk’s picture

hunmonk commented

you'd need to file an issue against core to address the problem in the way you describe.

drewish’s picture

drewish commented
Version: 5.x-1.x-dev » 4.7.x-2.x-dev

looks like hunmonk committed this to HEAD

aclight’s picture

aclight commented
Status: Patch (to be ported) » Fixed

We're not supporting the 4.7 branch anymore, so I'm closing this.

Anonymous’s picture

Anonymous (not verified) commented
Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.