Christian Schmidt [2007-07-12 17:08 UTC] It looks good. I am looking forward to getting OpenID support in PEAR.
Here are a few comments from reading the code. I haven't tried it out.
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.).
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.
Isn't the prefixing of method names with _ a pre-PHP5 convention?
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.
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.
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.
Can't the first half of Services_Yadis_Xrds::sortByPriority be replaced with ksort($unsorted) ?
In Services_Yadis_Xri::setXri(), the following looks wrong (always false):
"if (!strpos($xri, 'xri://') === 0 && ..."
Services_Yadis_Xri::toCanonicalId() can be optimized slightly by adding [last()] to the XPath expression.
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.
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?
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.
Pádraic Brady [2007-07-12 22:22 UTC] Hi Christian,
I'll get to your comments during tomorrow. All seem valuable, so I'll look into each when I have time to do it properly. On two of the points;
I underscore all private and protected methods and properties. It's not necessary per se but I find it a subtle hint that makes my reading at least that bit easier.
On the XRD namespaces. I'll double check, but I believe the XRD syntax requires the definitions as a reservation of sorts. Note this is broken by allowing users to override such reserved namespaces (as you commented) so something to be fixed.
Yeah, the meta regex is messy. One of those quick hacks to be replaced.
Thanks for the in-depth code review. Really helpful!
Pádraic Brady [2007-07-13 11:11 UTC] >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.
Christian Schmidt [2007-07-13 15:52 UTC] Just a few follow-up comments:
>I suggest using DOMDocument::loadHTML() in _isMetaHttpEquiv()
Cool. Shouldn't the http-equiv attribute values be treated as case-insensitive?
>I did some small changes in the Namespace object to refer to $namespaceKey.
Fine - though this is usually called "prefix".
>I've added a method to Services_Yadis_Service to allow the export
>of a DOMDocument
Nice. Instead of creating a whole new document, you can simply return a DOMElement like you do with SimpleXML using dom_import_simplexml($this->serviceNode). Perhaps getDomElement() is a better name to avoid confusion with the other classes in DOM API.
> The schema makes exclusive use of the "xrd" titled namespace
An XML document isn't required to use the same prefixes as in the XML document. This is an example of a document that validates against the schemas mentioned in the Yadis spec (at least with the validator, I used).
<?xml version="1.0" encoding="UTF-8"?>
<bar:XRDS
xmlns:bar="xri://$xrds"
xmlns:foo="xri://$xrd*($v*2.0)">
<foo:XRD>
<foo:Service>
<foo:Type>http://lid.netmesh.org/sso/2.0</foo:Type>
</foo:Service>
</foo:XRD>
</bar:XRDS>
>>In Services_Yadis::__construct() it is explicitly checked that the
>>second argument is an array. Is this necessary
>[...] it seems like a good idea to return them an early error about
>what parameter type is expected.
My point is that PHP does this automatically. If you supply anything else, PHP will trigger an error, e.g. "Catchable fatal error: Argument 1 passed to Services_Yadis::__construct() must be an array, integer given", and the exception will never be thrown (unless I missed something).
In Services_Yadis_Xrds_Namespace::addNamespace(), I think in_array() should be replaced with array_key_exists().
Pádraic Brady [2007-07-13 22:29 UTC] Ok, more patches to Subversion. Only thing unchanged is enforcing the namespace prefixes. I checked the Yadis library in Python and they're making similar checks - so it seems there is an implication "xrd" must exist. I might throw a post at the mailing list and see what the OpenID guys think.
|
