EALL Compliance for the CA Module

Fixed a spacing error. Use the new patch ryan.

I've set my test site to report E_ALL, and I don't see any notices on any of those pages. In fact, the only error I see is caused by a missing trigger on a custom predicate because I had reverted a change that had added one earlier.

I think E_ALL compliance is important, but I don't think this patch gets us any closer.

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.

As I go through the code, I am finding it more and more cumbersome to ensure E_ALL compliance. At some points, I take a leap of faith hoping that the calling function has already verified the data. This compliance was run on PHP 5.2 on MAMP. Other versions might not work.

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?

Drupal 7 will require PHP 5.2, so requiring it now will make updating the code that much easier. I thought we'd mentioned somewhere that we were only going to require 5.0, even though we're developing with 5.2.

I'm not particularly concerned about E_STRICT compliance. Part of the reason is that I have Devel set up to use Krumo for its backtrace messages, and it is not at all E_STRICT. This leads to a cascading infinite recursion of notices that crash my site. Hopefully this will be fixed by the time PHP 6 is done and E_ALL includes E_STRICT.

I agree with Ryan's 3rd point. Anyone who doesn't use an API correctly deserves the errors they get. Otherwise, they wouldn't necessarily know they are using it incorrectly. However, that does mean that we should build an API that is easy to use correctly and doesn't have errors when that happens.

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.

Unless this patch is causing errors, could we commit it? It will take several patches on the same files to bring ubercart to EALL Compliance. This is something that will take some time since the dataset is continually changing.

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