Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove empty path normalization to support base path use-case #487

Merged
merged 1 commit into from
Apr 7, 2015
Merged

Remove empty path normalization to support base path use-case #487

merged 1 commit into from
Apr 7, 2015

Conversation

Tobion
Copy link
Contributor

@Tobion Tobion commented Apr 3, 2015

This is to fix https://groups.google.com/forum/#!topic/php-fig/TswE4FD0SyM
It basically reverts #460 and clarifies things.

Also since we do not apply other normalization of the path as well (e.g. dot segment resolving), I think this is consistent.

@mtdowling
Copy link
Contributor

👍

1 similar comment
@nyamsprod
Copy link

👍

@weierophinney
Copy link
Contributor

Ironically, we chose to always return a / to enforce consistency (since the spec considers / and an empty path equivalent). I'm still not convinced that this is a good idea (I have yet to see a base path implementation that could not be adjusted to work with this, and my own experience with phly/conduit demonstrated that enforcing / on empty paths reduced a lot of boilerplate checks).

@Tobion I know that @simensen has shared a number of potential solutions with you, which you've brushed away as being insufficient, without demonstrating why. I'd like to see the why before we make a decision on this.

@mtdowling
Copy link
Contributor

Enforcing that paths be absolute basically removes the ability for a PSR-7 URI to represent a relative URI that can be combined accurately with a base URI.

For example, in the Guzzle PSR-7 library, I had to make the base URI resolution function accept a string as the relative URI to work around this: https://github.com/guzzle/psr7/blob/master/src/Uri.php#L115.

If I were to use a UriInterface object that always returns "/" for getPath(), then base URI resolution would never work with relative paths. For example: /foo/bar/ resolved with baz should result in /foo/bar/baz, but with the current PSR-7 spec would result in /baz because PSR-7 cannot represent relative URIs (due to the fact that getPath() always returns "/").

@weierophinney
Copy link
Contributor

@mtdowling Now, that is an excellent breakdown for me; thanks!

@pmjones and/or @simensen — go ahead and merge! (and note to self: a whole lot of rewriting in phly/conduit to do…)

simensen added a commit that referenced this pull request Apr 7, 2015
Remove empty path normalization to support base path use-case
@simensen simensen merged commit 5907c78 into php-fig:master Apr 7, 2015
@Tobion
Copy link
Contributor Author

Tobion commented Apr 10, 2015

what @mtdowling explained is exactly what I raided as concern. But his example goes even further and says, that UriInterface should also support relative (rootless in RFC 3986 terms) paths that do not start with a slash. Then we also need to adjust the __toString doc which currently says If a path is present, it MUST start with a "/" character.. This is of course not correct when when we support rootless paths. I only covered the empty path version in this PR.

@Tobion Tobion deleted the patch-2 branch April 10, 2015 00:17
@Tobion
Copy link
Contributor Author

Tobion commented Apr 10, 2015

I'll provide a PR to change that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants