Vote Details for "OpenID-rejected" by justinpatrin

» Details
» Comment
I'm +1 if the following problems are fixed before the first stable release. Returns and error handling must be fixed before any release in PEAR. The flat class tree is ok for alpha releases but must be fixed before the first beta release.

You *have* to use PEAR _Error (PEAR::raiseError()) for errors. This is
part of what PEAR standardizes, its error handling mechanism. This
will be a BC breaking release already due to all of the other changes
you're making, so changing the error handling should not be a problem.
In addition, PEAR_Errors can be "caught" as returns as well as through
global a handler (the same as trigger_error calls). If your previous
error handling was done through a global error handler you can still
do that with PEAR::setErrorHandling(PEAR_ERROR_CALLBACK,
'errorHandlingFunction');

Why do you the constant called Auth_OpenID_CURL_PRESENT? You can use a
PEAR call to check for curl availability and load the curl extension
if present.

What is the point of HTTPFetcher? It can't work as it is so why have 2
seperate classes. I would suggest implementing HTTPFetcher as a base
class and have sub-classes of HTTPFetcher_HTTPRequest and
HTTPFetcher_Curl. Have the HTTPRequest be the one used if Curl is not
found and use HTTP_Request (it can do redirects and all and it's
better to use shared code than implement this yourself).

You should not return mixed status and data info (I'm referring
specifically to HTTPFetcher::findIdentityInfo). On an error raise (and
return) a PEAR_Error. On success simply return the data. Users can
check for errors with PEAR::isError(). Errors should be handled like
this:
return PEAR::raiseError('error message', AUTH_OPENID_ERROR_SPECIFICERROR);

In addition, if you really want the script to DIE on a certain error
you can use:
return PEAR::raiseError('error message',
AUTH_OPENID_ERROR_SPECIFICERROR, PEAR_ERROR_DIE);
but this is really something that should only be done in the rarest of
circumstances.

In further addition, the info passed back by findIdentityInfo really
should be an associative array so that it's obvious which key is which
data. Such as:
return array('consumer_id' => $consumer_id, 'server_id' => $server_id,
'server' => $server);

Don't use s?printf unless you need some extended formatting. Using it
for simple %s replacement is wasteful. printf is far slower than
simple concatenation. Use 'some string '.$var.

The directories shouldn't be flattened quite as much as they are. You
currently only have one directory with quite a few files in it. Each
directory should have directly related classes (such as only
cryptography classes or only fetching classes). It's perfectly ok to
have, say:
Auth/OpenID/Crypto
and have 2 classes it it named
Auth_OpenID_Crypto_DiffieHellman
Auth_OpenID_Crypto_Blowfish
In some cases you don't even need a base class for them, although in
general you should have one which makes the interface apparent.
I'm sorry if this makes you un-do some of the flattening you've
already done, but this is the way that it really should be.

PHP files must be named after their classes. In addition, the names
should not be repeated. Currently in Interface.php you have the class
Auth_OpenID_OpenIDStore. This should be named Auth_OpenID_Store and be
in the file Auth/OpenID/Store.php. Then, adding to the above,
Auth_OpenID_SQLStore should be Auth_OpenID_Store_SQL and be in the
file Auth/OpenID/Store/SQL.php. The mysql store should be
Auth_OpenID_Store_Mysql and be Mysql.php next to SQL.php (it's ok to
have these in the same dir and yet have mysql extend sql).

is_subclass_of could (should?) be replaced with is_a.

This is not PEAR CS but it is encouraged to use all single quotes for
strings. It is easier to check code for errors and possible XSS
problems when variables are obviously added to a string instead of
hidden within one (as you can do with "). In addition, using ' is
faster than using " whenever you have variables in the string. You
don't have to just take my word for it, see my test results and try
running it yourself:
http://pear.reversefold.com/strings/
Of course, if you're using backslash expressions such as "\n" double
quotes are just fine.