Re: Authentication::OpenID Update

On 2/15/06, Jonathan Daugherty <cygnus@janrain.com> wrote: > Greetings, > > We've been hard at work making considerable changes to our OpenID > package to bring it into alignment with the feedback we've gotten. > Here's a list of things that we've fixed: > > * Functions have been moved into classes. These functions are now > just called as static class methods wherever they're needed. > > * The filesystem layout of the package has been modified to be more > consistent with class names. Directories like Consumer/ were > flattened. All files now live in Auth/OpenID/. > > * Classes now live in their own files rather than being grouped > together. > > * The usage of posix_getpwuid() in the detect.php script is now > conditional on its availability. > > In some earlier feedback we were asked about using PEAR error objects > instead of using the typical trigger_error method. For compatibility > purposes we're required to release a library that will not add a PEAR > dependency to applications that don't already require it. As such, we > couldn't use PEAR errors. Is there an alternative best practice in > this kind of situation? > Everything else sounds great, but this just isn't going to fly, sorry. 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'); > This snapshot also includes other improvements such as abstraction > changes, bugfixes, more example code, and some documentation updates. > Some things are incomplete (e.g. docblocks have not beed added for > everything), but I think this is a good time to get more feedback on > what we've done. > > Please take a look, and thanks to everyone who has spoken up! > > Package source: > > http://www.openidenabled.com/resources/downloads/php-openid/src/PHP-OpenID/EôGm9‘æÕf£ > NÂÊ > > Docs: > > http://www.openidenabled.com/resources/downloads/php-openid/src/doc/ > > Tar.gz: > > > http://www.openidenabled.com/resources/downloads/php-openid/src/PHP-OpenID.tar.gz > > Zip: > > http://www.openidenabled.com/resources/downloads/php-openid/src/PHP-OpenID.zip > Some more feedback: 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). (OT: HTTP_Request really *ought* to support a curl backend but it doesn't yet. If it did you wouldn't even need multiple backends here). 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. That's all I have at the moment. It looks like you've done a great job implementing all of this and I look forward to its inclusion in PEAR. -- Justin Patrin