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

Use the HTTP cookie header when available, instead of the super global #240

Closed
wants to merge 1 commit into from
Closed

Use the HTTP cookie header when available, instead of the super global #240

wants to merge 1 commit into from

Conversation

DASPRiD
Copy link
Member

@DASPRiD DASPRiD commented Mar 29, 2017

PHP will replace special characters in cookie names, which results in other cookies not being available due to overwriting. Thus, the server request should take the cookies from the request header instead.

* Parse a cookie header according to RFC 6265.
*
* PHP will replace special characters in cookie names, which results in other cookies not being available due to
* overwriting. Thus, the server request should take the cookies from the request header instead.

Choose a reason for hiding this comment

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

We ran into a problem with interacting with a Crowd SSO cookie which is named crowd.token_key but was getting set in $_COOKIE as crowd_token_key and requiring special handling to ensure that those two variants of the string match. This PR would solve that problem.

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.

Changes look good, and the test cases answer the issue nicely.

Would be useful to add some verbiage to the documentation indicating that headers are preferred over $_COOKIE when present, however, so folks know that behavior exists.

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.

One docs question...

for request parameters which were then registered as global variables. Due to this, cookies with a period
in the name were renamed with underlines. By getting the cookies directly from the cookie header, you have
access to the original cookies in the way you set them in your application and they are send by the user
agent.
Copy link
Member

Choose a reason for hiding this comment

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

Question: what happens if $_COOKIE is passed? fromGlobals() allows passing an array of cookies, and some of our documentation actually demonstrates passing in the superglobals explicitly. Should we discourage that behavior when it comes to cookies?

If so, we should likely add a note to that effect, and change some of the examples.

Copy link
Member Author

Choose a reason for hiding this comment

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

If $_COOKIE is passed there, you get the old behaviour. Not sure if we need to discourage it though. This is a rare case which this PR fixes, and usually people will call fromGlobals() without arguments I suppose?

Copy link

@jeremiahsmall jeremiahsmall Mar 31, 2017

Choose a reason for hiding this comment

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

Even if we don't actively discourage, it's probably worth adding a note stating what the expected result would be so that the mentioned examples are consistent with the new feature. @weierophinney is there a good search term or other way to locate the examples you are thinking of? I suppose fromGlobals and $_COOKIE would be a good place to start.

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.

Changes to the docs look good.

@weierophinney
Copy link
Member

@sharifzadesina Putting a thumbs-down as a reaction without providing context as to why is not terribly useful. Next time, please provide a comment if you disagree.

@weierophinney weierophinney added this to the 2.7.0 milestone Apr 6, 2017
@weierophinney
Copy link
Member

weierophinney commented Apr 6, 2017

Marking as milestone 1.4.0, as it's a feature change/addition, and could impact users who are not passing $_COOKIE when creating their request instance.

@weierophinney weierophinney modified the milestones: 1.4.0, 2.7.0 Apr 6, 2017
weierophinney added a commit that referenced this pull request Apr 6, 2017
Use the HTTP cookie header when available, instead of the  super global

Conflicts:
	CHANGELOG.md
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, @DASPRiD

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.

3 participants