Port must be an integer and other small improvements in Uri #321
Conversation
snapshotpl
changed the title
There was a problem hiding this comment.
Similar to #319, this poses a BC break, as it disallows previously allowed values that might be an artifact of manually parsing a string URL in order to obtain the port.
We likely should NOT allow float values, as that would be lossy when we cast to int; however, string int values SHOULD be allowed currently.
src/Uri.php
Outdated
| @@ -333,17 +335,13 @@ public function withHost($host) | |||
| */ | |||
| public function withPort($port) | |||
| { | |||
| if (! is_numeric($port) && $port !== null) { | |||
| if (! is_int($port) && $port !== null) { | |||
There was a problem hiding this comment.
This is problematic, as the port value may be derived from a string. This is why the check was originally for is_numeric, and we cast to int. While PHP's parse_url() does the right thing, and casts the discovered port to an integer, the value might come from another location:
- from
$_SERVER, in which case it's a string - by splitting the authority portion of the URI, in which case it's a string
- by manually parsing the URI in order to provide a more robust solution than
parse_url(), in which case casting would be up to the parser
Requiring an int at this point is a BC break.
test/UriTest.php
Outdated
| $this->assertSame($uri, $new); | ||
| $this->assertEquals('3001', $new->getPort()); | ||
| $this->assertEquals(3001, $new->getPort()); |
There was a problem hiding this comment.
Internally, since we cast to an integer already, the above line should become:
$this->assertSame(3001, $new->getPort());That change should be made regardless of accepting the other portions of this patch.
test/UriTest.php
Outdated
| @@ -159,22 +158,23 @@ public function testWithPortReturnsNewInstanceWithProvidedPort($port) | |||
| public function testWithPortReturnsSameInstanceWithProvidedPortIsSameAsBefore() | |||
| { | |||
| $uri = new Uri('https://user:pass@local.example.com:3001/foo?bar=baz#quz'); | |||
| $new = $uri->withPort('3001'); | |||
| $new = $uri->withPort(3001); | |||
There was a problem hiding this comment.
The fact that this line must change is an indication of the BC break.
test/UriTest.php
Outdated
| 'string' => [ 'string' ], | ||
| 'string-int' => [ '3000' ], | ||
| 'array' => [ [ 3000 ] ], | ||
| 'object' => [ (object) [ 3000 ] ], |
There was a problem hiding this comment.
This one is invalid, and needs to be changed regardless of other changes. It should read:
'object' => [ (object) ['port' => 3000]],
|
@weierophinney can we keep this changes to v2.0? |
I'd rather not. Since the spec itself doesn't have explicit scalar type hints defined, what the constructors and various If/when we do an update to the PSR-7 specification to add scalar type hints, we can revisit. |
|
👍 |
Port must be an integer and other small improvements in Uri
|
Thanks, @snapshotpl! |
No description provided.