Vote Details for "HTTP_OAuth" by shupp
| » Details |
|---|
|
| » Comment |
|
Jeff, Great job overall on this. I think the change to HTTP_Request2 alone will greatly help adoption, and your debug console in examples is outstanding. However, my vote is conditional for a few reasons: * The package.xml is borked - you have a tgz file in it that doesn't exist, which suggests that you didn't package this up and test it via PEAR before calling for votes. * The test coverage on the provider classes is very low, and not acceptable for where you and I work (and who sponsored its development). Please revisit this. * Dependencies should be explicit. Please use PHP_CompatInfo to get the specific dependencies (hash is missing, for example). * The tgz link on the proposal doesn't work for me. I had to checkout and build the package (after fixing the package.xml), and then install it to test. Here are specific notes for other issues I found: HTTP_OAuth * Use 'helper' instead of 'help' in class doc block * Does the logs array need to be public? I figured it would be protected. * Why is there no detach() method for log observers? Observers should be able to be attached and detached. HTTP_OAuth_Consumer * getRequestToken() throws an exception, but its doc block is missing @throws * getAccessToken() same as above HTTP_OAuth_Exception * Why is __construct() defined? It appears to duplicate the signature of PEAR_Exception, which it simply calls HTTP_OAuth_Message * The class doc block references the consumer and provider packages which are no longer separate (they were initially) * IMO, the parameter handling is a little confusing when reading the code. i.e. setParameters() getParameters(), __get() and __set(). * Doc blocks in the class are a bit sparse * getIterator() is not tested. Please add a test for it. HTTP_OAuth_Consumer_Request * Attaching your log instances to HTTP_Request2_Observer_Log is pretty brilliant. Nicely done. * Rethrowing HTTP_OAuth_Exception in 212 does not retain the value of the caught Exception. If you want to rethrow, at least use $e->getMessage(), $e->getCode(). HTTP_OAuth_Consumer_Response * getResponse() doc block refers to HttpMessage, which appears to be out of date * Same thing for __call() HTTP_OAuth_Provider_Request * Class doc block is just the class name. It needs more. * setParametersFromRequest() is missing @throws annotations HTTP_OAuth_Provider_Response * Class doc block is just the class name. It needs more. * Do you really need to wrap headers_sent()? Seems unnecessary. HTTP_OAuth_Signature * factory doc block is missing @throws annotations HTTP_OAuth_Signature_Common * Weak doc block descriptions. They mostly just restate the method names as words. HTTP_OAuth_Signature_HMAC_SHA1 * Has an undeclared dependency on the hash extension. I've also seen other packages check that the algorithm (sha1 only in this case) is available via hash_algos() before relying on it. * Twitter's /account/verify_credentials is complaining about using POST, it says GET is required (in your debug consule) Lastly, while its awesome you've included detailed examples in your proposal description, there's an error in the consumer sample code: * Missing trailing ' in first argument of getRequestToken() |