E_ALL compliance: 1- Use of if (isset($var)) or if (!empty($var))

I would tend to agree with you... I was merely following the suggestion made on the devel list.
People have been submitting patches for E_ALL for ages, in different issues, but none of them have been committed yet.
Is Dries against this whole idea, or is he waiting for us to do something we haven't done yet?
I followed the suggestion because all else has failed previously, so I was thinking that maybe that was what was missing: proper, clear guidelines, etc...

If a one liner in the handbook saying "Write E_ALL compliant code" is enough, I'd be very happy, because we'd be able to delete the handbook page I wrote, I wouldn't have to keep it up to date, etc.., and I'll get back to writing patches so that they get submitted ASAP.

So, what do you think would increase our chances to see our E_ALL patches committed?

I have this one, but it needs review and possibly changes (I'm still learning how to do things):
http://drupal.org/node/28540#comment-49935

Meanwhile, could anyone comment on the patch attached to this issue:
1- is the diff format the proper one? Is it commitable?
2- is the code change appropriate?
if the answer to both is YES, please mark this issue as patch-> ready to be committed.
if not, tell me what I am doing wrong so I can learn.

Thanks.

12 more instances of the same.

re-roll to latest head.

the same, updated.

I started this thread because I belong to the group of developers who would like to see Drupal fully E_ALL compliant.

Drupal and E_ALL compliance

There are three groups of developers.

1. Many would like Drupal to be E_ALL compliant.
2. Most can't be bothered and won't help with this issue without actually being against.
3. Few if any are categorically against E_ALL compliance ('bloated code'?).

It's up to the first group of people to make it happen.
The problem is manyfold:

1. Regardless of Drupal's code numerous qualities, it is a sad mess with regard to declaring variables: there is a lot of mess to clean!
2. More mess is being added everyday as patches and new functionality are being committed to core. So, if we want to clean Drupal of E_ALL notices, we need to have fixes committed faster than new sources of E_ALL notices are being introduced
3. There is no clear vision or understanding among developers (of the first group) on how to actually solve the problem.
4. There is a lot to fix, but at the same time, large patches are very unlikely to get committed (especially lacking serious review). The best we have achieved so far is a only line patch being committed (#6) and Dries set the issue as 'fixed' (??). Way to go!! ;)

Note that the purpose of this thread was precisely to figure out the way to solve the problem.

Important note on methodology

I still hope that Jaza's patch can get committed but it has little chance because of its size and even less lacking serious review by trusted core developers (i.e. not me). More importantly, not every one agrees on the exact fix (see above #11, #13, #19).

Note the link to the handbook page at the top of this issue: our first aim is to create a very definite policy with regard to coding style and how to solve error notices in different situations. Once such a coding policy has been set and agreed by the core commiters, they then can (and should) refuse a patch if it doesn't comply with the policy (which should solve the second of my list of problems above).

Since the errors are all over the place, we also need to proceed methodologically.
Here is the procedure I propose to follow:

1. Make a clean install of Drupal cvs.
2. Create your admin account then go straight to admin > modules and turn OFF all non-required modules, leaving us with a bare-bone install.
3. In includes/common.inc change the line if ($errno & (E_ALL ^ E_NOTICE)) { to : if ($errno & (E_ALL)) which will allow you to experience the mess :)
4. Click on the admin > settings link and start debugging the notices, one at a time, starting from the first one on the list!
5. Get a fix for the first error notice committed.
6. Get a coding policy relevant to that first fix firmly establish and enforced by core commiters.
7. Rince and repeat.
8. ???
9. ...
10. $$$!!

First notice on the list

Following the procedure outlined above, the first notice on admin/settings is:
notice: Undefined index: #processed in includes/form.inc on line 396.
which appears 5 times on this page.

The attached patch is the same fix as proposed in Jaza's patch above.
Crell has a point, though: who has a better fix?

Our first notice seems to be a rather arcane one...

http://api.drupal.org/api/HEAD/file/developer/topics/forms_api_reference...
says:

#processed :
INTERNAL. Used to ascertain whether or not a form element has been processed (ie: expanded to multiple elements).

The key '#processed' occurs only three times in the whole Drupal code.

Many other notices are about undefined array indices.
I don't see any place in the code where the form is being initialized with default values given for each index.

As Crell pointed out, maybe a more comprehensive solution would be to enforce a declaration of the variables earlier on in the code.
I'm not familiar with form.inc. Can someone help out?

I tried adding '#processed' => FALSE) in modules/system.module:
$type['checkboxes'] = array('#input' => TRUE, '#process' => array('expand_checkboxes' => array()), '#tree' => TRUE, '#processed' => FALSE);
but it didn't work.

It seems that form_builder() IS the place I was looking for to initialize the form elements, at least in this case.
The new patch is an alternative way to fix the same problem as in patch #20. #20 is safer, as I am sure it won't break anything. This one needs proper review, but may be a better solution.

Comments?

Yes. see my important, longish comment #20.
The topic of this issue is precisely to agree on how to make that happen.
patch #15 is currently out, because it is long, will probably never be properly reviewed, and there no agreement yet on how to fix the issues.

The patch needing review now is the one in #22.
Patch #20 fixes exactly the same problem but with a different approach, so #20 and #22 can be compared.

As per Crell's good comment #19, I'd say +1 for patch #22.
If others agree, that's the one to commit.

Hmm. Sorry for the corrupt patch file.
I tested this one and it works.
(otherwise, it's the same as #22 above).

why fixed?
There are many more to go :)

For the record the #26 approach (declaring a default value early on) won over that of #20 (use of isset()).

Comment in #20 still applies.

next on admin/settings is:

notice: Undefined offset: 0 in includes/form.inc on line 886.

The same error appears 10 times on that page, i.e. once for each radio setting.

We're back to the use of isset(). I don't see a better solution now, but there could be.

Here is the unmodified block of code in question:

/**
 * Roll out a single radios element to a list of radios,
 * using the options array as index.
 */
function expand_radios($element) {
  if (count($element['#options']) > 0) {
    foreach ($element['#options'] as $key => $choice) {
      if (!$element[$key]) {
        $element[$key] = array('#type' => 'radio', '#title' => $choice, '#return_value' => $key, '#default_value' => $element['#default_value'], '#attributes' => $element['#attributes'], '#parents' => $element['#parents'], '#spawned' => TRUE);
      }
    }
  }
  return $element;
}

I don't see other solutions for this one, because the code relies on the key NOT existing in order to create the proper radio element in the form.

The form makes use of the key #spawned which is NOT documented in the form api reference:
http://api.drupal.org/api/HEAD/file/developer/topics/forms_api_reference...
#spawned is not used anywhere else in the code.