E_ALL Compliance: Fix PHP Notices in project.module
plumbley - December 17, 2006 - 13:29
| Project: | Project |
| Version: | 5.x-1.x-dev |
| Component: | Projects |
| Category: | bug report |
| Priority: | minor |
| Assigned: | Unassigned |
| Status: | closed |
Description
project.module 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. Additional
<?php
$output = '';
?>in
project_page_overview(), that uses $output .= ... later.
2.
<?php
if ($_POST['edit']['rid']) {
?>changed to
<?php
if (!empty($_POST['edit']['rid'])) {
?>3.
<?php
if ($project->release_count > 1) {
?>changed to
<?php
if (isset($project->release_count) && $project->release_count > 1) {
?>4.
<?php
if (module_invoke($module, 'project_sort_methods', 'group by date', $sort_method) && $date = _project_date($project->changed)) {
?>changed to
<?php
+ if (isset($module) && module_invoke($module, 'project_sort_methods', 'group by date', $sort_method)
&& $date = _project_date($project->changed)) {
?>(Actually this indicates some other possible problem
project_page_overview(), since presumably $module ought to exist at this point. So maybe this should be left as a bug to be fixed).
5.
<?php
$output .= '<div class="project" id="project-overview">' . $projects . '</div>';
?>changed to
<?php
$output .= '<div class="project" id="project-overview">' . (isset($projects) ? $projects : '') . '</div>';
?>although an alternative would be to show an alternative message if no projects were listed.
Best wishes,
Mark.
| Attachment | Size |
|---|---|
| project.module_5.patch | 4.38 KB |
#1
(Setting correct status)
#2
here's a re-roll for 5
#3
this is definitely wrong:
@@ -708,7 +709,7 @@'href' => "project/issues/$project->nid",
);
}
- if (module_invoke($module, 'project_sort_methods', 'group by date', $sort_method) && $date = _project_date($project->changed)) {
+ if (isset($module) && module_invoke($module, 'project_sort_methods', 'group by date', $sort_method) && $date = _project_date($project->changed)) {
$projects .= "<h3>$date</h3>";
}
$project->class = ($class == 'even') ? 'odd': 'even';
what are you doing that's resulting in $module not being set? that doesn't look possible to me. if it is, then we need to fix elsewhere.
#4
@hummonk - Sorry this is so long ago now that I don't remember what I did exactly. But I only ever started to try out Project, so this was probably either (1) browse of projects with no projects created yet, or (2) creation of first project. I think (2) is most likely, since most node add/edit forms ask for something about the (assumed to exist) item, but on create it doesn't exist yet so instead returns NULL node with PHP Notice.
A clean bare installation would be the most likely place to see this I think, since creating the first of a particular type can sometimes be a boundary case that isn't always catered for well.
#5
the above mentioned weirdness w/ $module has been fixed elsewhere, so rip that out and roll a new patch against the latest code.
#6
this should do what we need. quick review would be nice.
#7
Visually looks fine and light testing on a local site reveals no problems. Didn't actually turn on E_ALL yet, so it's possible there are other warnings lurking, but this is certainly an improvement. Thanks.
#8
i turned on E_ALL and found a few more. we might as well get them all at once.
#9
- $version_form = drupal_get_form('project_release_version_filter_form', $version);+ $version_form = drupal_get_form('project_release_version_filter_form');
something's wrong here. that form isn't built anywhere else that i can see, so what's the deal with
function project_release_version_filter_form($version = NULL)if we're removing $version in the patch? do we need to get rid of $version as an arg, or correctly supply $version infunction project_page_overview()?#10
#11
reviewed, tested, committed to 5.x-1.x -- leaving to be ported if anybody cares to fix this in the older versions.
#12
#13