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

Prefix authority with double slash if present. #235

Closed
wants to merge 1 commit into from

Conversation

ajgarlag
Copy link
Contributor

@ajgarlag ajgarlag commented Feb 22, 2017

The PSR-7 docblock of the UriInterface::__toString method, states that:

If an authority is present, it MUST be prefixed by "//".

This PR tests and fixes that behavior.

Copy link

@asgrim asgrim left a comment

Choose a reason for hiding this comment

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

Although having read through the interface and looking at the Guzzle implementation I agree that this patch fixes an issue. However, it does introduce a BC break (as some things may rely on this already not having // prefixed)

public function testAuthorityIsPrefixedByDoubleSlashIfPresent()
{
$uri = (new Uri())->withHost('example.com');
$this->assertEquals('//example.com', (string) $uri);
Copy link

Choose a reason for hiding this comment

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

This test is exactly the same as testUriDoesNotAppendColonToHostIfPortIsEmpty except the URL used... not sure we need both except for being explicit.

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 was unsure too.

@ajgarlag
Copy link
Contributor Author

Every bug fixed is a sort of BC break as you cannot know if there are things that relay on the old behavior, but for me it should be fixed in order to be PSR7 compliant.

@sharifzadesina
Copy link
Contributor

sharifzadesina commented Mar 23, 2017

@ajgarlag why you have closed this PR?

@ajgarlag ajgarlag reopened this Mar 23, 2017
@ajgarlag
Copy link
Contributor Author

@sharifzadesina It was an error,
is open again!

Copy link
Member

@weierophinney weierophinney left a comment

Choose a reason for hiding this comment

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

I tend to agree that this is bugfix that ensures that the behavior follows the documented expectations of PSR-7. That said, I'll push it to the 1.4.0 release so we can message it more clearly.

@weierophinney weierophinney added this to the 1.4.0 milestone Apr 6, 2017
weierophinney added a commit that referenced this pull request Apr 6, 2017
Prefix authority with double slash if present.
weierophinney added a commit that referenced this pull request Apr 6, 2017
weierophinney added a commit that referenced this pull request Apr 6, 2017
@weierophinney
Copy link
Member

Merged to develop for release with 1.4.0. Thanks, @ajgarlag

jesseschalken added a commit to ivt/zend-diactoros that referenced this pull request May 15, 2017
zend-diactoros 1.4.0

Added
-----

- [zendframework#219](zendframework#219) adds two new
  classes, `Zend\Diactoros\Request\ArraySerializer` and
  `Zend\Diactoros\Response\ArraySerializer`. Each exposes the static methods
  `toArray()` and `fromArray()`, allowing de/serialization of messages from and
  to arrays.

- [zendframework#236](zendframework#236) adds two new
  constants to the `Response` class: `MIN_STATUS_CODE_VALUE` and
  `MAX_STATUS_CODE_VALUE`.

Changes
-------

- [zendframework#240](zendframework#240) changes the
  behavior of `ServerRequestFactory::fromGlobals()` when no `$cookies` argument
  is present. Previously, it would use `$_COOKIES`; now, if a `Cookie` header is
  present, it will parse and use that to populate the instance instead.

  This change allows utilizing cookies that contain period characters (`.`) in
  their names (PHP's built-in cookie handling renames these to replace `.` with
  `_`, which can lead to synchronization issues with clients).

- [zendframework#235](zendframework#235) changes the
  behavior of `Uri::__toString()` to better follow proscribed behavior in PSR-7.
  In particular, prior to this release, if a scheme was missing but an authority
  was present, the class was incorrectly returning a value that did not include
  a `//` prefix. As of this release, it now does this correctly.

Deprecated
----------

- Nothing.

Removed
-------

- Nothing.

Fixed
-----

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

Successfully merging this pull request may close these issues.

None yet

4 participants