• notice: Undefined index: n in sites/all/modules/storm/stormknowledgebase/stormknowledgebase.module on line 98.
  • notice: Undefined index: n in sites/all/modules/storm/stormnote/stormnote.module on line 112.

    • notice: Undefined index: n in sites/all/modules/storm/stormorganization/stormorganization.module on line 108.

  • notice: Undefined index: n in sites/all/modules/storm/stormperson/stormperson.module on line 152.
  • notice: Undefined index: n in sites/all/modules/storm/stormproject/stormproject.module on line 180.
  • notice: Undefined index: n in sites/all/modules/storm/stormtask/stormtask.module on line 167.
  • notice: Undefined index: n in sites/all/modules/storm/stormteam/stormteam.module on line 210.
  • notice: Undefined index: n in sites/all/modules/storm/stormticket/stormticket.module on line 171.
CommentFileSizeAuthor
#29 storm--tests.patch14.91 KBjuliangb
#22 storm--1042698-22.patch89.64 KBjuliangb
#18 storm--1042698-18.patch93.74 KBjuliangb
#16 storm--1042698-16.patch93.72 KBjuliangb
#14 storm--1042698-14.patch53.6 KBjuliangb
#12 storm--1042698-12.patch24.56 KBjuliangb
#10 storm--1042698-10.patch23.31 KBjuliangb
#8 storm--1042698-8.patch3.84 KBjuliangb
#3 storm.warnings.patch42.94 KBpillarsdotnet
storm.rewrite_where_sql.patch5.78 KBpillarsdotnet

Comments

juliangb’s picture

juliangb commented

Could we accompany this with some tests to ensure that the errors are not reintroduced in the future?

pillarsdotnet’s picture

pillarsdotnet commented

@juliangb -- Yup. Go right ahead.

pillarsdotnet’s picture

pillarsdotnet commented
StatusFileSize
new storm.warnings.patch42.94 KB

lotsa-lotsa-lotsa undefined index errors thoughout the entire module. I care about PHP warnings, but I'm starting to lose steam after committing 12 hours to what should have been a 10-minute install.

juliangb’s picture

juliangb commented

@pilllarsdotnet, I also care about getting Storm free of PHP warnings, however, we need to structure the approach to patching in order that the fix remains and the effort is not wasted.

The way that we have done these so far is through adding tests - albeit some quite simple. These flag PHP warnings as exceptions, meaning that all future patches have to keep the same standards.

Whilst I will look at these patches for committing, it would be much more effective if you could collaborate on some of the following issues.
#918748: Tests for Storm Expense
#918752: Tests for Storm Team
#918750: Tests for Storm Timetracking

There are tests already for a number of the Storm modules, and so for these we can add lines that call particular pages - hence exposing any PHP warnings that appear when a user views those pages.

juliangb’s picture

juliangb commented

Would it be possible to post a list of the paths of pages where you are seeing warnings?

I think then it will be easier to combine your patch with a few tests.

pillarsdotnet’s picture

pillarsdotnet commented
Status: Needs review » Needs work

The only pages I tested were the sub-pages of the main "Storm" menu entry: (from memory), Organizations, People, Teams, Tickets, Tasks. And I ran out of motivation *way* before I ran out of errors. I don't use Storm, and I'm done for now with the customer who does.

To duplicate, set errorlevel to E_ALL and check the logs. If the original coders did this, then the errors would not have occurred in the first place. Sorry, but the following construct is annoyingly amateurish and ubiquitous, especially in Storm modules:

  $foo = (something possibly undefined)
  if (!$foo) {
    $foo = (something provably defined)
  }

I haven't yet learned simpletest; sorry. I imagine you could just visit all of the defined module pages and check for dblog entries labeled "PHP" but I dunno.

rjgoldsborough’s picture

rjgoldsborough commented

I was seeing many of these while working on #918748: Tests for Storm Expense and #918752: Tests for Storm Team, and I believe most of them are fixed with those two patches. If not all, a good deal of them at least.

juliangb’s picture

juliangb commented
Title: Multiple undefined index notices in *_rewrite_where_sql() functions. » Ensure all pages defined by Storm are tested
Category: bug » task
Status: Needs work » Needs review
StatusFileSize
new storm--1042698-8.patch3.84 KB

The way I'm handling this is to add tests that call each page that Storm defines. This will show up any PHP warnings as exceptions. I'll then use the above patches to help solve those.

I'll post a few work in progress patches to ensure tests pass.

Status: Needs review » Needs work

The last submitted patch, storm--1042698-8.patch, failed testing.

juliangb’s picture

juliangb commented
Status: Needs work » Needs review
StatusFileSize
new storm--1042698-10.patch23.31 KB

Status: Needs review » Needs work

The last submitted patch, storm--1042698-10.patch, failed testing.

juliangb’s picture

juliangb commented
Status: Needs work » Needs review
StatusFileSize
new storm--1042698-12.patch24.56 KB

Status: Needs review » Needs work

The last submitted patch, storm--1042698-12.patch, failed testing.

juliangb’s picture

juliangb commented
Status: Needs work » Needs review
StatusFileSize
new storm--1042698-14.patch53.6 KB

Status: Needs review » Needs work

The last submitted patch, storm--1042698-14.patch, failed testing.

juliangb’s picture

juliangb commented
Status: Needs work » Needs review
StatusFileSize
new storm--1042698-16.patch93.72 KB

Status: Needs review » Needs work

The last submitted patch, storm--1042698-16.patch, failed testing.

juliangb’s picture

juliangb commented
Status: Needs work » Needs review
StatusFileSize
new storm--1042698-18.patch93.74 KB
pillarsdotnet’s picture

pillarsdotnet commented

There are a lot of places where you change:

if ($foo) {

to:

if (isset($foo)) {

If this follows a statement like:

$foo = 0;

then obviously if (isset($foo)) will evaluate to if (TRUE) whereas if ($foo) will evaluate to if (FALSE).

In other words, your patch is based on the assumption that no tested variable is ever set to a value like FALSE, 0, or an empty string. They all are always either undefined, NULL, or set to a TRUE value.

I find it hard to reconcile such complete confidence in the code with the necessity of a 94-kilobyte patch.

juliangb’s picture

juliangb commented

I looked at each change in its own merit -> therefore in some cases the change was the addition of an isset(), in other cases different methods of solving.

I should have also put a footnote, I have not added additional tests for Stormtimetracking or Stormteam. The former already has an issue looking at this, and the latter as there are changes going through which I'd like to get sorted first.

I'm going to leave this patch in the queue for a few days in case of query. Feel free to ask for clarification on specific changes if you have questions.

juliangb’s picture

juliangb commented

#18: storm--1042698-18.patch queued for re-testing.

juliangb’s picture

juliangb commented
StatusFileSize
new storm--1042698-22.patch89.64 KB

Reroll using git.

rfay’s picture

rfay
he,him
Primary language English
Location Palisade, CO, USA
commented
Status: Needs review » Needs work

OK, just putting this in "needs work" to see if that will stop the continual retesting.

juliangb’s picture

juliangb commented
Status: Needs work » Needs review

#22: storm--1042698-22.patch queued for re-testing.

juliangb’s picture

juliangb commented
Version: 6.x-2.x-dev » 6.x-1.x-dev

Committed to 6.x-2.x.

Retest on 6.x-1.x.

juliangb’s picture

juliangb commented

#22: storm--1042698-22.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, storm--1042698-22.patch, failed testing.

juliangb’s picture

juliangb commented

As I expected, will need a reroll for 1.x.

juliangb’s picture

juliangb commented
Assigned: Unassigned » juliangb
Status: Needs work » Needs review
StatusFileSize
new storm--tests.patch14.91 KB

This patch should apply against 1.x - contains the test files. Hence should deliberately fail.

Status: Needs review » Needs work

The last submitted patch, storm--tests.patch, failed testing.

juliangb’s picture

juliangb commented
Version: 6.x-1.x-dev » 6.x-2.x-dev
Assigned: juliangb » Unassigned
Status: Needs work » Fixed

Considering the roadmap for moving priorities over to 2.x., I'd suggest we don't spend effort on migrating these tests to 1.x., and call this fixed now.

The tests are included in 2.x, so ongoing work and bug fix patches already in effect take account of them.

Status: Fixed » Closed (fixed)

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