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

Add cookies to the client (https://github.com/atoum/AtoumBundle/issues/5... #52

Closed
wants to merge 4 commits into from

Conversation

Rebolon
Copy link

@Rebolon Rebolon commented Oct 21, 2013

...1)

@Rebolon
Copy link
Author

Rebolon commented Oct 21, 2013

Ok i launch it

@Rebolon
Copy link
Author

Rebolon commented Oct 21, 2013

This PR solve a part of the problem from issue #51

@nchaulet
Copy link
Contributor

👍

1 similar comment
@stephpy
Copy link
Member

stephpy commented Oct 22, 2013

👍

@jubianchi
Copy link
Member

:shipit:

@FlorianLB
Copy link
Member

What about adding $cookies as an option to the createClientmethod ?

@jubianchi
Copy link
Member

@FlorianLB +1

@Rebolon
Copy link
Author

Rebolon commented Oct 22, 2013

@FlorianLB so the new createClient method would look like this :

public function createClient(array $options = array(), array $server = array(), array $cookies = array())

I think it's a cool thing.
I change it now

@FlorianLB
Copy link
Member

Seems good for me.

@jubianchi
Copy link
Member

👍

@FlorianLB
Copy link
Member

Can you squash your commits ? I will merge this PR after.

@Rebolon
Copy link
Author

Rebolon commented Oct 22, 2013

What is the aim of this squash ? won't we loose the history of this PR because i have to push on a new branch and do a new PR, am i wrong ?

@FlorianLB
Copy link
Member

In the commit history we don't need to see the different iterations of a PR. For a "simple" change like this one, only one commit is perfect.

You can rebase (and squash your commits during) on this branch, no need a new branch/PR.

If you can't do this, it doesn't matter.

@FlorianLB
Copy link
Member

Thx :)

@FlorianLB FlorianLB closed this Oct 22, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants