[PEPr] Comment on Authentication::OpenID_Consumer

Christian Schmidt (http://pear.php.net/user/schmidt) has commented on the proposal for Authentication::OpenID_Consumer. Comment: It looks good. Here are some comments based on reading the code. Don't worry - most are minor nits :-) I suggest grouping the OPENID_ constants with a common prefix, e.g. OPENID_MODE_NO_ENCRYPTION and OPENID_MODE_STATELESS instead of OPENID_NO_ENCRYPTION and OPENID_STATELESS. There is a lot of different concepts in use here. This makes the code harder to understand and use for outsiders who haven't implemented OpenID before. To make life easier for new users, I suggest making method names very explicit (most are already very explicit), and include example values in the documentation of @param and @return, e.g. use "johndoe.openidprovider.example.org" every time a userIdentifier is used. When the return value is a string constant, I suggest mentionin this, e.g for OpenID::getMode() "@return string OPENID_STATELESS or OPENID_STATEFULL" or (if you follow the above suggestion) something like "@return string an OPENID_MODE_* constant". I suggest renaming OPENID_PHP_SESSION_NAMESPACE to e.g. OPENID_PHP_SESSION_KEY to avoid confusion with XML-like or OpenID-like namespaces (if I understand it correctlyk, the OpenID namespaces have nothing to do with XML namespaces?). OpenID::setAssocType() takes as argument either 'sha1' or 'sha256', but OpenID::getAssocType() return either 'HMAC-SHA1' or 'HMAC-SHA256'. Is there any reason for that? It breaks the Principle of least surprise. I suggest only using constants for session types and assoc types. This will make setSessionType() and setAssocType() simpler. This should also allow you to get rid of preg_match'ing on assoc types in OpenID::getHashFromAssociation(). The method name of Association::getAssociation() implies that it returns an Association, but it returns a KVContainer. Is there a difference between "assoc type" and "association" and "association type"? If not, I suggest using the same term everywhere. In Association::_isXri(), $identifier[0] will trigger an error, if $identifier is "" (I don't know if that can actually happen). Perhaps Association::_isXri() belongs in Services_Yadis_Xri instead? At least it shouldn't be duplicated in Consumer::_isXri(). Association::_getCachedAssociation states that it returns "@return bool", though it returns an OpenID_KVContainer. In Association::_getHash(), the result of OpenID::getVersion() is compared to a string, though it is a float. KVContainer::__toString() should use PHP_EOL instead of \n (according to the OpenID spec). In KVContainer::__set() I recommend ensuring that keys do not contain \n or : and that values do not contain \n. In KVContainer::_parseString(), the checks "count($pairs) == 0" and "is_array($pairSplit)" can be removed (explode() always returns an array with size >= 1). Is it permitted to trim trailing whitespace off the values? There are references to Zend_Service_Yadis, Zend_Http_Response and "@category Zend". I assume these are left-overs from when you proposed the package to the Zend Framework. In DiscoveryHtml::parse(), you should probably mute $html->loadHTML() (e.g. with @) to prevent it from making noise because of bad HTML. In DiscoveryHtml::parse(), the search for <link> elements could perhaps be done easier using XPath. In Consumer::setOpEndpoint(), any trailing / in the URI is stripped. Is this really allowed? Some methods, e.g. OpenID_Association::getSharedSecret, use both camelCase and underscore_style for variables. Is there any In Redirect_Authorisation::getUri() - is it possible for getOpEndpoint() to return a URL already containing a "?" ? In that case, "&" should be appended instead of "?". In finish Consumer::finish(), a Response_Exception is caught only to be thrown again. There is no reason for that (is there?). If Consumer::setUserIdentifier() is called with the string "foo.example.org/bas", it is translated into ""http://foo.example.org/bas/". On the other hand, "http://foo.example.org/" and "http://foo.example.org" are equivalent, and should be normalized to the same URL. Further normalization is probably overkill at this stage, but if you are going to work on it, I suggest you do it in the Net_URL2 package (see bug #11574 about making this more compliant to RFC3986 that is mentioned in the OpenID spec). Proposal information: http://pear.php.net/pepr/pepr-proposal-show.php?id=500 -- Sent by PEPr, the automatic proposal system at http://pear.php.net