Fix issue #66 #67
Fix issue #66 #67
Conversation
|
Can you add a test to cover this scenario? |
|
Hi @jwage, thank you for your reply. I've added the mapped unit test for this issue. |
|
I don't understand what this is fixing. The hashbang is removed? |
tests/Purl/Test/UrlTest.php
Outdated
|
|
||
| // test fragment with colon | ||
| $url = new Url('http://example.com/#hello:123'); | ||
| $this->assertEquals('http://example.com/', (string) $url); |
There was a problem hiding this comment.
@jwage, yes. It's fixed that issue. Please see more details about that.
It has removed the hash bang.
There was a problem hiding this comment.
This doesn't feel right to me. It shouldn't just silently remove it from the URL.
There was a problem hiding this comment.
What do you think about this improvement?
I think we can fetch this hash bang and store another key in Url instance.
What do you think?
There was a problem hiding this comment.
Are hashbangs technically allowed to have a colon in them?
There was a problem hiding this comment.
I don't think that hash bang with colon is not the standard.
There was a problem hiding this comment.
Then I am not sure if there is anything to fix here. Dropping the hashbang value entirely is not a good fix. I would maybe accept an enhancement to allow the hashbang with the colon like you described but that would be a more involved fix.
There was a problem hiding this comment.
Consider the approach:
- parse the url.
- get the fragment.
- validate the fragment whether it's with the colon.
What do you think? Thanks.
src/Purl/Fragment.php
Outdated
| @@ -109,7 +109,16 @@ public function __toString() : string | |||
| protected function doInitialize() : void | |||
| { | |||
| if ($this->fragment !== null) { | |||
| $this->data = array_merge($this->data, parse_url($this->fragment)); | |||
| $pos = strpos($this->fragment, ':', 1); | |||
There was a problem hiding this comment.
Hi @jwage, I use the strpos function to get the first position of the colon character.
And sub the string with colon then return the current url with the valid fragment.
There was a problem hiding this comment.
I think we misunderstood each other. It should maintain the whole hashbang. i.e. #hello:123
There was a problem hiding this comment.
You're right. The hash bang value is the #hello:123.
Perhaps we can rebuild the fragment value after using the parse_url function.
What do you think?
There was a problem hiding this comment.
Yes. We would need to parse out the hashbang manually instead of relying on parse_url since it can't handle it when it has a colon on it.
| @@ -109,7 +111,17 @@ public function __toString() : string | |||
| protected function doInitialize() : void | |||
| { | |||
| if ($this->fragment !== null) { | |||
| $this->data = array_merge($this->data, parse_url($this->fragment)); | |||
| $pos = strpos($this->fragment, ':', 1); | |||
There was a problem hiding this comment.
Here is my approach:
- Detect the hash bang whether it's with the colon.
- Rebuild the hash bang with the colon after using the
parse_urlfunction.
There was a problem hiding this comment.
@peter279k: Are we sure that this is the only case that would cause parse_str to fail ? Why not just use parse_str right away, and fallback to $data = ['path' => $this->fragment] if it fails ?
It would make less assumptions about how parse_str works now and in the future. This would make this code more robust.
Changed log
Try to fix issue #66.