Skip to content
This repository has been archived by the owner on Jan 31, 2020. It is now read-only.

URI with ending colon without port #34

Merged
merged 9 commits into from Oct 7, 2019
Merged

URI with ending colon without port #34

merged 9 commits into from Oct 7, 2019

Conversation

mtagliab
Copy link
Contributor

Based on issue #33

Previous behaviour

$sourceUri = "http://www.example.com:";
$uri = new \Zend\Uri\Http($sourceUri);

echo 'Scheme: '.$uri->getScheme().PHP_EOL;
echo 'Host: '.$uri->getHost().PHP_EOL;
echo 'Port: '.$uri->getPort().PHP_EOL;
echo 'Path: '.$uri->getPath().PHP_EOL;

Which gives the following result:

Scheme: http
Host: www.example.com:
Port: 
Path: /

New behaviour
With this fix, the new result will be:

Scheme: http
Host: www.example.com
Port: 
Path: /

which matches the result of parse_url($uri).

I also added a new test for this case.

@mtagliab mtagliab changed the title Develop URI with ending colon without port Sep 20, 2019
@michalbundyra michalbundyra added this to the 2.7.1 milestone Sep 20, 2019
@mtagliab
Copy link
Contributor Author

@webimpress I fixed the error with the last commit. I did all the work on a test machine, but couldn't commit from there, and I rewrote the modifications on web. During the process, I somehow forgot to remove the setPort outside the check, causing the test to fail. Should be correct now

@michalbundyra
Copy link
Member

@mtagliab Thanks, I'll have a look on it shortly.

Copy link
Member

@michalbundyra michalbundyra left a comment

Choose a reason for hiding this comment

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

@mtagliab Thanks for your contribution, it looks good, just two small comments.

src/Uri.php Outdated
$this->setPort((int) $port);
// If authority ends with colon, port will be empty string.
// Remove the colon from authority, but keeps port null
if ($port && $port !== '') {
Copy link
Member

Choose a reason for hiding this comment

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

if ($port) is sufficient here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know, but I preferred keeping the strict compare. I'll change it right now

test/UriTest.php Outdated
$uri = new Uri($uriString);

$this->assertSame($uri->getHost(), 'www.example.com');
$this->assertSame($uri->getPort(), null);
Copy link
Member

Choose a reason for hiding this comment

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

please swap arguments - first is EXPECTED and the second is ACTUAL

@michalbundyra
Copy link
Member

@mtagliab Also, the target branch should be master, not develop. Could you rebase it and change the target, please?

@mtagliab
Copy link
Contributor Author

@webimpress I never rebased a repo. What should I do, exactly?

@michalbundyra
Copy link
Member

@mtagliab

I never rebased a repo. What should I do, exactly?

git rebase origin/master and then push force. On github you can edit PR and change the target from develop to master. If you have any issues I can do it on merge, not a problem. Thanks.

@mtagliab mtagliab changed the base branch from develop to master September 30, 2019 11:45
@mtagliab
Copy link
Contributor Author

mtagliab commented Oct 7, 2019

@webimpress could it be merged now?

@michalbundyra
Copy link
Member

@mtagliab I need to review first, will try to do it today

michalbundyra added a commit that referenced this pull request Oct 7, 2019
URI with ending colon without port
michalbundyra added a commit that referenced this pull request Oct 7, 2019
michalbundyra added a commit that referenced this pull request Oct 7, 2019
@michalbundyra michalbundyra merged commit 1cb32be into zendframework:master Oct 7, 2019
@michalbundyra
Copy link
Member

Thanks, @mtagliab!

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

Successfully merging this pull request may close these issues.

None yet

2 participants