Vote Details for "OpenID-rejected" by justinpatrin

» Details
Voter: Justin Patrin [Wishlist] Vote: +1 (conditional) Reviews: Cursory source review
» 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.