Re: Net_URL Bug #6470

Andrei Railean wrote:

I agree that Net_URL should be the place where any url preparation and parsing is to take place for the whole of PEAR.

That's not what I said.

On 07/02/2006, at 5:24 PM, bertrand Gugger wrote:

I wonder in how many places in pear the same, or at least approaching, goals of validating/correcting URIs is done. We have currently the same discussion in Text_Wiki about the best way to encode complient URLs. You can also take a look in the uri() method from Validate. http:// cvs.php.net/viewcvs.cgi/pear/Validate/Validate.php

Not sure if URI validation belongs in Validate package. It seems that it belongs inside the Net_URL as that is the place where most of the knowledge about compliancy and validity of URLs is contained. Validate module could then simply interface with Net_URL to validate the string in question. But that will make the Validate module heavier, which may not be so good in some cases.

Which will never occur. Validate must be standalone.

However, the main point of discussion is about Net_URLs methods for ensuring RFC compliance.

Anyway, I agree the path should be url encoded. About your patch: - why use preg_split ? only to get no empty parts ? Is it not yet done be done by the method resolvePath() ? More generally, I think everything should be done there.

resolvePath() says that its purpose is this: "Resolves //, ../ and ./ from a path and returns", so I didn't want to touch it. It is a static function that does not get used inside Net URL itself so I didn't want to break other classes using it. resolvePath may be modified to accept an array of options with an flag to make the path RFC compliant. Something like Array ('rfc_encode' => true) Then we could encode the path in resolvePath, but the function name will be confusing. Maybe have a function 'preparePath' that will run resolve it as well as encode the components.

Right, it's better BC this way.

preparePath could be used inside getURL to return the prepared path, but that may require a few flags as well. Not sure whether the desired output of getURL is the properly encoded RFC compliant path, or simply an aggregation of all of the components the way they were supplied in the constructor plus substitution of those that were missing. I can see the use for both, but if we take the RFC definition of a URL, then URL should be compliant on return from getURL function call. This is because if it is not compliant, it is not a URL, but merely a string that wants to be a URL.

- as urlencode treats spaces as +, I wonder if rawurlencode() would not be better complient (%20), but remember *all* not alphanum nore '-_.' will be, perhaps unnecessarly, encoded (!~*\'()$+&,...)

I agree, rawurlencode() is better.

- you accept lonely '%' in path, which I believe not rfc2396 complient - '?' in path ?

my bad. copy and paste. will correct in the next patch submision. I'll come up with the regex to parse the path in such a way that only % hex hex go through, but not lonely %.

- on the countrary I believe '\' *is* allowed

RFC says that it belongs to 'unwise' characters and is disallowed:

unwise      = "{" | "}" | "|" | "\" | "^" | "[" | "]" | "`"

section 2.4.3.

Which construct is never used in the BNF for URI, so it's blahblah. Anyway, my bad, "\" is never appearing in the allowed chars, I checked again and tested Validate::uri() , we agree, I just had bad read back this lovely regexp :)

- looks like it treats, or should treat only absolute path, is it ?

It assumes the $this->path starts with a '/', check the constructor.

$this->path        = !empty($HTTP_SERVER_VARS['PHP_SELF']) ?  $HTTP_SERVER_VARS['PHP_SELF'] : '/';

If $HTTP_SERVER_VARS['PHP_SELF'] is set, it will have a '/' in it, I believe.

This is done only if the parameter $url is not some absolute url. The affectation for path is further, and I'm unsure what it does. That looks a little complicated to get that $this->path is empty... and I'm unsure dirname() always return a path starting with a "/" in every situation on all machines. We would need some "guru" here ... lol

- more generally, it's unclear how and where this method will be called, or I miss something...

As the bug report says, this method is to be used in places where direct access to the path property is currently taking place. HTTP_Request is one particular example.

OK, I understand, it's an extra method.

I would really like some unified method in pear for that, so we don't repeat everywhere the same thing and more important that we act coherently.

In conclusion, it appears that my patch was premature so I'll fix the points mentioned above and supply another patch. However, I would like to hear the feedback on the points discussed above, particularly on the extension of resolvePath or introduction of preparePath.

That does not look so bad ! +1 for an extra method which would itself call resolvePath() , but as said, I don't belong to gurus here, better ask them. ( I belong to stupids ) Regards -- toggg