Skip to content

Conversation

@andrewnester
Copy link
Contributor

@csev
Copy link

csev commented Jul 11, 2017

Thanks so much - that was really quick.

@csev
Copy link

csev commented Jul 11, 2017

Just curious - I would have checked the length of ZSTR_VAL(url->s) before grabbing ZSTR_VAL(url->s)[0] :) Little I know of course.

@andrewnester
Copy link
Contributor Author

andrewnester commented Jul 11, 2017

@csev the whole condition is url_parts->fragment && '#' == ZSTR_VAL(url->s)[0].
In case if length of url->s will be 0, url_parts->fragment will obviously be false.
So I guess no need to add one more check for this.

smart_str_append_smart_str(dest, url);
php_url_free(url_parts);
return;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: incorrect indentation

@nikic
Copy link
Member

nikic commented Jul 11, 2017

Did not try, but http://php.net#foo might be another problem case.

@andrewnester
Copy link
Contributor Author

@nikic what is the expected output for this case? http://php.net?PHPSESSID=sessionidhere#foo or http://php.net#foo ?

@nikic
Copy link
Member

nikic commented Jul 11, 2017

It should be http://php.net/?PHPSESSID=sessionidhere#foo. There's code handling the !path&&!query case, but it assumes fragment is empty.

@andrewnester
Copy link
Contributor Author

@nikic exactly! thanks, I've just updated my PR according to comments

@krakjoe krakjoe added the Bug label Jul 12, 2017
@andrewnester
Copy link
Contributor Author

@nikic @krakjoe any updates on this?

@krakjoe
Copy link
Member

krakjoe commented Jul 18, 2017

I'm going to assign it to nikita and ask that he merge it if he's happy with it ... he's touched and or reviewed this code recently if my memory serves me correct (which it may not) ...

@nikic
Copy link
Member

nikic commented Jul 18, 2017

Merged as 6c32d27, thanks!

@nikic nikic closed this Jul 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants