[PEPr] Comment on Web Services::Services_Yadis

Pádraic Brady (http://pear.php.net/user/padraic) has commented on the proposal for Web Services::Services_Yadis. Comment: >I suggest using DOMDocument::loadHTML() in _isMetaHttpEquiv() to make it more robust. Parsing >HTML with regexps is a hack that will work in many cases, but not all (e.g. if the page uses >single quotes for enclosing attribute values, if the content attribute appears before the >http-equiv attribute, if it has obsolete <meta> elements removed using HTML comments tags etc.). I have switched that section to using DOMDocument now. Also, while not yet proposed, I corrected the same thing in the OpenID Consumer code. >Though it is a personal opinion, I think public methods should return DOMElements rather >than SimpleXMLElements. SimpleXML makes a developer's life easier, if he knows what he is >doing, but if he isn't familiar with the API, he will easily get confused (using var_dump(), >is_array(), is_string(), isset() etc. often returns surprising results). SimpleXML-fans >can easily convert to SimpleXML using simplexml_import_dom() (and vice versa, I know). Or >the API can supply both a getSimpleXmlObject() and a getDOMElement() method for convenience. I've added a method to Services_Yadis_Service to allow the export of a DOMDocument using getDomObject(), and switched the name of getXmlObject() to getSimpleXmlObject(). Most of the internal logic is structured around SimpleXML so I'd prefer to leave the internal handling alone (a lot of extra work with little return). In general, since Yadis is quite a restricted "do it my way or else" protocol it's unlikely people need more than Services_Yadis_Service::getTypes(), getUris(), getPriority(), getElements(); and the internal SimpleXML handling is relatively basic as is. > Isn't the prefixing of method names with _ a pre-PHP5 convention? Not that I'm aware of. It's probably a java-esque practice but I always underscore private and protected things so they're more easily spotted inside the source code without having to look up the comments. >The term "namespace" is used in variable and method names to refer to 1) instances of >Services_Yadis_Xrds_Namespace, 2) namespace URIs, and 3) namespace prefixes. I suggest >using the terms "namespaceObject" (or simple) "namespace"), "namespaceUri" and "prefix", >respectively. I did some small changes in the Namespace object to refer to $namespaceKey. Might make it clearer to anyone reading that class where the variables fit in. I think the public method names are okay but I'll update the comments if needed. >Some methods are "@return string|null". At least in the native PHP API, the convention is >that string functions return false rather than null in cases where a string cannot be >returned. I don't know if PEAR conventions differ. Have switched to returning false. >Sometimes input to the Services_Yadis_Exception constructor is HTML encoded using >htmlentities(). This breaks the convention from PEAR_Exception that the input is plain >text. Unfamiliar with PEAR_Exception really. I checked, and it does appear to call htmlspecialchars() on any input messages. I'll do some checking later to make sure it's safe enough to remove the htmlentities() call I'm using. Not that I'm implying PEAR_Exception is insecure :). Just not familiar with its use. >Can't the first half of Services_Yadis_Xrds::sortByPriority be replaced with >ksort($unsorted) ? It's a classic. You do a function, make some adjustments, refactor it a little, and completely miss the fact you just reinvented ksort() at some point. :) >In Services_Yadis_Xri::setXri(), the following looks wrong (always false): >"if (!strpos($xri, 'xri://') === 0 && ..." Good catch! >Services_Yadis_Xri::toCanonicalId() can be optimized slightly by adding [last()] to the >XPath expression. Done. Need to finish this function later. I left it half-done since OpenID 2.0 was still in flux at the time and, well, only a tiny number of people actually have an XRI. Poor excuse maybe :). >In Services_Yadis_Xrds::_getValidXrdNodes() it is enforced that the XRDS document uses >a specific prefix ("xrd") for the XRDS namespace and that the default namespace is a >specific namespace. This seems too strict. Do these restrictions really apply to XRDS >documents? I don't know, but that would seem like an unusual requirement for an XML >document. In XML documents authors can generally choose the prefixes freely. Quote (Section 7.5, Yadis Specification 1.0): "The schemas of the Yadis document are the XRDS and XRD schemas contained in this clause. A Yadis document MUST be a valid XML document and MUST conform to the XRDS schema. A Yadis Resource Descriptor is an XRD element; it MUST be contained in an XRDS element and MUST conform to the XRD schema." The schema makes exclusive use of the "xrd" titled namespace so seems enough to impose it as a strict rule. I'll do some more digging to be 100% on this - I'm guessing the OASIS Specifications for XRI probably make it a bit clearer. >In Services_Yadis::__construct() it is explicitly checked that the second argument is an >array. Is this necessary when the type is specified in the function's parameter list? The parameter is optional to start with - one can just addNamespace() anything they need once the object is instantiated. But at construction, the constructor will call addNamespaces() plural, so while it's not necessary it seems like a good idea to return them an early error about what parameter type is expected. >What is the purpose of allowing the user to register his own namespaces using >Services_Yadis::addNamespace() or Services_Yadis::__construct() rather than letting >him register them afterwords directly on the SimpleXMLElement (or DOMXPath)? The current >aproach has the (purely theoretic) disadvantage that a user has to know that the prefixes >"xrd" and "xrds" are reserved and must not be used. The "xrd" and "xrds" namespaces are now protected and can't be reset. Additional namespaces are sometimes required for non-standard elements. For example, OpenID 1.1 defines an openid:Delegate element (so someone needs to register "openid" as a namespace before running any XPath query for it using, say, Services_Yadis_Service::getElements('openid:Delegate'). Part of the reason for handling XML Namespaces in such an abstract way (besides reuse) is to eliminate the need for direct use of the SimpleXMLElement or DOMDocument objects. You can get any required data for the Yadis protocol using the Services_Yadis_Service class API, or the single high level method Services_Yadis::getCanonicalID (since the CanonicalID is only one level past the root so it's unrelated to services and serves as an XRI Identities unique non-transferable ID). Needing the XML objects themselves yields little extra benefit - though it's of course allowable for anyone extending Yadis. Changes noted above are now in subversion - I'll update the proposal package (which I just noticed has a few bad MD5 file hashes anyway!) in a while. Proposal information: http://pear.php.net/pepr/pepr-proposal-show.php?id=499 -- Sent by PEPr, the automatic proposal system at http://pear.php.net