E_ALL Compliance for the CA Module

Fixed a spacing error. Use the new patch ryan.

E_ALL has different values for various versions of PHP. Likewise, E_STRICT is not included in E_ALL and applies only for PHP 5. So as part of your examination of these issues for Ubercart, perhaps we should start with a clear statement of what minimum version of PHP Ubercart will support - right now it's been left pretty fuzzy ... I'm all for requiring PHP 5.2 or above, regardless of whether Drupal pretends to support PHP 4. I don't see that "E_ALL" compliance has any meaning unless it's tied to a definite version of PHP.

I think it's important to give the code a cleansing, although this is mainly necessary because of shortcomings in the PHP language. I also think the issue of E_ALL in Drupal coding standards http://drupal.org/node/34341 needs to be worked on, because there's a lot of ambiguity in the current proposal.

Good points raised by TR. At this point, for UC 2, I think we were settling on PHP 5.2 b/c of our ImageCache dependence for OOB image support. I'll need Lyle to corroborate, though, because I suppose we never wrote it anywhere. Our .info files do state PHP 5.0 at the very least.

Also, a couple things to keep in mind when making code compliant:

1. Constants should be capitalized, therefore null should be NULL. See the naming conventions at http://drupal.org/coding-standards.
2. When variables are declared at the beginning of functions, introduce a blank line after them so there's separation between variable declaration and the meat of the function. This patch does this in some places it seems but not in others.
3. Take into consideration whether or not existing APIs will ensure that variables are set. For example, many of the lines in this patch are devoted to making sure the various keys exist in things like $predicate arrays. The API function ca_predicate_load() will make sure that these keys exist by virtue of how it's building the predicate array. Someone could write a module that didn't conform to the API specifications, so maybe I'm understanding this point wrong, but I don't think Drupal generally panders to people who use APIs improperly. (An example would be the t() function which doesn't do validation to ensure the second argument is an array.) If it results in a bunch of extra code and inline if statements, I'd rather not pander to them either.

Feedback from anyone?

OK, now I have to apologize, because I figured out why I wasn't seeing any E_NOTICEs. I had set the drupal_error_handler() to use E_ALL, but the backtrace_error_handler() in devel.module was still set to E_ALL ^ E_NOTICE. So, yes, ca.admin.inc needs fixing.

The patch may not cause errors, but it needs to be rerolled to address my points in #5.

I just found a couple of E_ALL errors (notices) and wanted to share... I see that someone already got a few fixes here.

In regard to using NULL instead of null and other such things, you probably want to use the coder project to check your code before submitting.

I'm submitting my patch because I did not see the same things fixed in the other patch...

Thank you.
Alexis

@AlexisWilke: This issue is still listed as "needs work". If you can fix up the previous patch from #1 to address the issues in #5 and to add your additional fixes from #11, then it will be ready for review. Then all we will need is someone to give it a good test to make sure it addresses all the E_ALL issues in the CA module. So far, it doesn't seem that anyone has interest enough in this issue to do that work.

I'm not too keen on having to process dozens of E_ALL patches. I would rather have one big one which takes care of all the issues, excepting only those changes which are non-trivial. That's why I went through the issue queue three months ago and tagged the E_ALL issues. As I've said in other threads, if you're willing to do the work to roll the patches, address any concerns, and get the patches reviewed, I'm willing to commit your patches.

Hi TR,

There is a patch. It's only 12,000 lines, about 47Kb.

I always follow the Drupal coding rules so this will look correct as requested by #5.

Also, I very much agree with point (3) of #5, it is important to NOT avoid E_NOTICE errors of invalid data. If someone does not define the title or name of something when that parameter is mandatory, the E_NOTICE will appear. Much better!

I ran Coder against the files I'm patching so a few of the changes are Coder changes and not directly E_ALL related. Coder formatting is just a few spaces added/removed.

Thank you.
Alexis

TR,

As I was making an update and going through all the changes I made to keep them... I wanted to underline the following which is a bug. So I'm posting here again to push this issue back to the top since E_ALL does help you in fixing real bugs. I hope you'll have the time to look into it.

Thank you.
Alexis

Here is a review, just to checking for anything that stood out while reading through the patch, I haven't actually applied or tested it.

Why is the key changed here, is this some other bug?

Doesn't seem to make much sense checking for NULL, only to just set it NULL if it is NULL

NULL?

NULL?

What is this: NULL /* = time() */

Does this change also require an update as well? No need to leave this line in with that comment, should just remove the line.

The problem with consolidating all this logic is trying to read it - some extra comments might help here. Also you are removing the unset() which is effecting the logic in this function.

This doesn't feel quite right... is there not a way to ensure the variables contains the correct contents to start with?

Same as above.

NULL again.

NULL

I personally prefer something like this written as:

For two reasons, less lines of code and it just feels better to define the variables outside the if context when the intension is to use it outside that context.

There are a lot of lines which have the format of " = isset(something) ? something : NULL;", whats the difference between just saying "= something" (this could be some quirk of PHP I'm not aware of?). If the effort is taken to check all these values, then why not give it a meaningful default then just NULL?

Also just to clarify, none or very few of these may need changing, all I have done is make some comments against changes to try and understand things that are not obvious.

Powered by Dreditor.

Alexis, I understand this is about E_ALL which is exactly why I was specifically looking for any places where you potentially change the logic and not just fix an E_NOTICE. As I said I didn't apply or test the functionality of the patch and I didn't write any of this code myself, all I have done was provided a review of the contents of the patch to help you move the issue forward - I would suggest that you can stop the condescending comments like "(which is why any PHP programmer should have E_NOTICE turned on!)", its starting to get old with all the different issues you have posted this and I will get tied as I'm sure other will of bothering to help with this issue. Lets just keep the issue on topic and fix it.

I'll admit I didn't have the comments from #17 in my mind when reviewing the code, so my first issue is no logger a problem as you have explained that - although you could make this issue easier to fix by separating any bug fixes you find in their own issues as this is supposed to be focussed on E_ALL compliance (I understand you found the issue due to E_ALL, but its not an E_ALL problem that is a logic error where an index is being used that doesn't exist). By committing this patch as is there will continue to be questions about whether that change will break something else and that change might also get better exposure with its own issue.

The NULL lines probably comes from my background as a C programmer where having variables undefined (NULL) is a potential memory leak issue, so I personally don't think NULL should be used as a valid value set by any variables (isn't that also going to be a potential issue for throwing other E_NOTICE messages elsewhere?), its just not the best programming practice in my mind to leave variables undefined - if this is going to be fixed, then we may as well apply a fix thats not going to possibly throw more E_NOTICE's. The bigger thats causing these errors is really the poor typing in PHP.

I guess the issue I see with those array merges in uc_ups is those operations are done every time you require those variables - and array merging is not a cheap operation in most programming language (to be honest I have not investigated if PHP has some efficient way that other languages don't), but my thought is would it not be better to add the values to the variable on installation and also provide an update to add those values for any sites that don't have them set. I'm sure doing it that way will also have a small performance benefit over the current proposed patch.

I believe most of these PHP notices in ca have been fixed by now. The only way for me to be sure is to have people test, so I'm consolidating all the open PHP notice issues.