Skip to content
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

[6.x] Add methods for sending cookies with test requests #30101

Merged
merged 1 commit into from Sep 26, 2019

Conversation

@jasonmccreary
Copy link
Contributor

commented Sep 25, 2019

This introduces methods for making it easier to send cookies in HTTP Tests. Previously, sending cookies restricted developers to using the low-level call method and required manually encrypting the cookie values (#12032).

Before

$cookies = [
    'name1' => encrypt('value1'),
    'name2' => encrypt('value2')
];
$response = $this->call('get', 'test', [], $cookies);

After

$response = $this->withCookies([
    'name1' => 'value1',
    'name2' => 'value2'
])->get('test');

Similar to the header methods, the following methods were added:

  • withCookie() for setting a single cookie with a value.
  • withCookies() for setting multiple cookies with name/value pairs.
  • disableCookieEncryption() for disabling the automatic encryption of cookie values.

Cookies are prepared before sending the HTTP request and encrypted using the encrypter.

This implementation implies a few design decisions:

  • To remain backwards compatible, the call method does not honor cookies set by these new methods. In a future version, all logic could be moved to the call method and merge its $cookie argument with cookies set by these methods.
  • Since the EncryptCookies middleware is only enabled for web requests by default, the json* HTTP methods do not honor cookies set by these new methods. This could be amended by passing the unencrypted cookie values in these methods.
  • Since cookies values are not serialized by default starting in Laravel 5.5, these methods do not serialize cookie values. This could be amended by adding a enableCookieSerialize method or leveraging the static property of EncryptCookies.

In the end, these methods improve the out-of-the-box developer experience by making it easier to test requests using cookies. For more complex scenarios, developers can continue using call as they have been all along.

@jasonmccreary jasonmccreary force-pushed the jasonmccreary:with-cookie branch from 61fa912 to aceda01 Sep 25, 2019
@jasonmccreary jasonmccreary force-pushed the jasonmccreary:with-cookie branch from aceda01 to 4542387 Sep 25, 2019
@jasonmccreary

This comment has been minimized.

Copy link
Contributor Author

commented Sep 25, 2019

These tests are pretty weak and could likely be removed. They only verify internal properties. My attempts to test sending the actual cookies required mocking out parts of this internal trait which was not pretty.

If there is some kind of echo or loopback test request I can make let me know and I will gladly add more tests.

@gocanto

This comment has been minimized.

Copy link
Contributor

commented Sep 26, 2019

would I need to call disableCookieEncryption() if I am using my own encrytion? - if so, it would be nice to detect it?

protected function prepareCookiesForRequest()
{
if (! $this->encryptCookies) {
return $this->defaultCookies;

This comment has been minimized.

Copy link
@gocanto

gocanto Sep 26, 2019

Contributor

what would happen here if I needed my own cookies?

This comment has been minimized.

Copy link
@jasonmccreary

jasonmccreary Sep 26, 2019

Author Contributor

This method only effects the code if you use these new cookie methods and one of the HTTP method helpers. As noted in the PR, if you have your own encryption/cookies, you can continue testing as you have before using call directly.

This comment has been minimized.

Copy link
@gocanto

gocanto Sep 26, 2019

Contributor

It makes sense, thanks for your reply.

I am still a bit lost with the implementation tho. For instance, if I can withCookies, should I expect to send the default ones too?.

Is it expected to remove the default?

This is my main doubt by looking at the changes.

@driesvints driesvints changed the title Add methods for sending cookies with test requests [6.x] Add methods for sending cookies with test requests Sep 26, 2019
/**
* Define additional cookies to be sent with the request.
*
* @param array $cookies

This comment has been minimized.

Copy link
@driesvints

driesvints Sep 26, 2019

Member
Suggested change
* @param array $cookies
* @param array $cookies
/**
* Add a cookie to be sent with the request.
*
* @param string $name

This comment has been minimized.

Copy link
@driesvints

driesvints Sep 26, 2019

Member
Suggested change
* @param string $name
* @param string $name
* Add a cookie to be sent with the request.
*
* @param string $name
* @param string $value

This comment has been minimized.

Copy link
@driesvints

driesvints Sep 26, 2019

Member
Suggested change
* @param string $value
* @param string $value
@driesvints

This comment has been minimized.

Copy link
Member

commented Sep 26, 2019

@jasonmccreary I fixed the build on the 6.x branch. Can you rebase? The build will pass then.

@taylorotwell taylorotwell merged commit 4542387 into laravel:6.x Sep 26, 2019
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/styleci/pr The analysis has passed
Details
@jasonmccreary jasonmccreary deleted the jasonmccreary:with-cookie branch Sep 26, 2019
@jasonmccreary

This comment has been minimized.

Copy link
Contributor Author

commented Sep 26, 2019

@driesvints looks like this was merged. Yay! Let me know if you want me to make any additional changes. I'll open a patch PR.

@driesvints

This comment has been minimized.

Copy link
Member

commented Sep 26, 2019

@jasonmccreary no worries. Build on 6.x passes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.