New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make array value cookies able to be sent. #11209

Merged
merged 3 commits into from Sep 19, 2017

Conversation

Projects
None yet
4 participants
@markstory
Member

markstory commented Sep 19, 2017

Add a new method to get a cookie value as a string. I considered changing how getValue() works but thought that had bigger compatibility implications. Instead I've modified the interface/implementation. Normally I would shy away from changing interfaces, but this is a very new class and the likelihood of there be user-space implementations is quite low.

Refs #11208

markstory added some commits Sep 19, 2017

Remove redundant doc blocks.
Inherit documentation from the interface where possible.
Add Cookie::getStringValue()
Add a new method to get a cookie value as a string. I considered
changing how `getValue()` works but thought that had bigger
compatibility implications. Instead I've modified the
interface/implementation. Normally I would shy away from changing
interfaces, but this is a very new class and the likelihood of there be
user-space implementations is quite low.

Refs #11208

@markstory markstory added the http label Sep 19, 2017

@markstory markstory added this to the 3.5.3 milestone Sep 19, 2017

@markstory markstory referenced this pull request Sep 19, 2017

Closed

Issues with the new Cookie management #11208

1 of 3 tasks complete
@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Sep 19, 2017

Codecov Report

Merging #11209 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #11209      +/-   ##
============================================
+ Coverage     93.14%   93.15%   +<.01%     
- Complexity    12981    12983       +2     
============================================
  Files           437      437              
  Lines         33626    33630       +4     
============================================
+ Hits          31321    31328       +7     
+ Misses         2305     2302       -3
Impacted Files Coverage Δ Complexity Δ
src/Http/Cookie/Cookie.php 99.4% <100%> (+0.01%) 60 <2> (+2) ⬆️
src/Http/Response.php 93.67% <100%> (ø) 257 <0> (ø) ⬇️
src/Cache/CacheRegistry.php 100% <0%> (+4%) 11% <0%> (ø) ⬇️
src/Cache/CacheEngine.php 93.61% <0%> (+4.25%) 19% <0%> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 173293f...6cf84bc. Read the comment docs.

codecov-io commented Sep 19, 2017

Codecov Report

Merging #11209 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #11209      +/-   ##
============================================
+ Coverage     93.14%   93.15%   +<.01%     
- Complexity    12981    12983       +2     
============================================
  Files           437      437              
  Lines         33626    33630       +4     
============================================
+ Hits          31321    31328       +7     
+ Misses         2305     2302       -3
Impacted Files Coverage Δ Complexity Δ
src/Http/Cookie/Cookie.php 99.4% <100%> (+0.01%) 60 <2> (+2) ⬆️
src/Http/Response.php 93.67% <100%> (ø) 257 <0> (ø) ⬇️
src/Cache/CacheRegistry.php 100% <0%> (+4%) 11% <0%> (ø) ⬇️
src/Cache/CacheEngine.php 93.61% <0%> (+4.25%) 19% <0%> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 173293f...6cf84bc. Read the comment docs.

*
* @return string
*/
public function getStringValue();

This comment has been minimized.

@jeremyharris

jeremyharris Sep 19, 2017

Member

Any reason not to use __toString?

@jeremyharris

jeremyharris Sep 19, 2017

Member

Any reason not to use __toString?

This comment has been minimized.

@markstory

markstory Sep 19, 2017

Member

Cookies have more properties than just the value. Wouldn't the string value of a cookie include the Secure, HttpOnly, Path and Domain bits?

@markstory

markstory Sep 19, 2017

Member

Cookies have more properties than just the value. Wouldn't the string value of a cookie include the Secure, HttpOnly, Path and Domain bits?

This comment has been minimized.

@jeremyharris

jeremyharris Sep 19, 2017

Member

Yep, you're absolutely right! Disregard :)

@jeremyharris

jeremyharris Sep 19, 2017

Member

Yep, you're absolutely right! Disregard :)

* Gets the cookie value
*
* @return string|array
* {@inheritDoc}
*/
public function getValue()
{
return $this->value;

This comment has been minimized.

@stefanozoffoli

stefanozoffoli Sep 19, 2017

I applied your fix and it correctly save the cookie now.
However when I read the cookie value, it continues to be in json format.

Can be correct to modify here like this?

return $this->_expand($this->value);

But also when using ServerRequest->getCookie($cookie_name); it returns the cookie value in json format and it doesn't call Cookie->getValue().

@stefanozoffoli

stefanozoffoli Sep 19, 2017

I applied your fix and it correctly save the cookie now.
However when I read the cookie value, it continues to be in json format.

Can be correct to modify here like this?

return $this->_expand($this->value);

But also when using ServerRequest->getCookie($cookie_name); it returns the cookie value in json format and it doesn't call Cookie->getValue().

This comment has been minimized.

@stefanozoffoli

stefanozoffoli Sep 19, 2017

By setting

return $this->_expand($this->value);

There are warnings in the EncryptedCookieMiddleware so it's more complicated than this 😅

Also if you have the EncryptedCookieMiddleware active it already return the value in array format without the need to modify getValue()

@stefanozoffoli

stefanozoffoli Sep 19, 2017

By setting

return $this->_expand($this->value);

There are warnings in the EncryptedCookieMiddleware so it's more complicated than this 😅

Also if you have the EncryptedCookieMiddleware active it already return the value in array format without the need to modify getValue()

This comment has been minimized.

@markstory

markstory Sep 19, 2017

Member

The value of the cookie is the JSON data though when you're reading from a request. The cookie middleware adds more behavior to cookies though. I wouldn't recommend people store JSON data in plaintext cookies as its full of 💣

@markstory

markstory Sep 19, 2017

Member

The value of the cookie is the JSON data though when you're reading from a request. The cookie middleware adds more behavior to cookies though. I wouldn't recommend people store JSON data in plaintext cookies as its full of 💣

@markstory markstory merged commit c65346c into master Sep 19, 2017

6 checks passed

codecov/patch 100% of diff hit (target 93.14%)
Details
codecov/project 93.15% (+<.01%) compared to 173293f
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
stickler-ci No lint errors found.

@dereuromark dereuromark deleted the issue-11208 branch Oct 23, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment