Skip to content

Conversation

@dbu
Copy link
Contributor

@dbu dbu commented Jan 2, 2019

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Related tickets -
Documentation -
License MIT

What's in this PR?

Went over the code again to find places where we can add type hints.

* @throws TransferException
*/
private function createCookie(RequestInterface $request, $setCookie)
private function createCookie(RequestInterface $request, string $setCookieHeader)
Copy link
Contributor

Choose a reason for hiding this comment

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

: ?Cookie?

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we'd need PHP 7.1 for that. Is there any reason why 2.x is php 7.0+ and not 7.1+. 7.0 is EOL already.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thats why i did not do it. but i wondered the same. happy to move to 7.1+. wdyt @Nyholm ?

Copy link
Member

Choose a reason for hiding this comment

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

I agree. Lets do PHP7.1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cool! however, i'd prefer to do that in a separate pull request, as it will entail quite a lot of changes.

@dbu dbu added this to the v2.0.0 milestone Jan 3, 2019
@dbu
Copy link
Contributor Author

dbu commented Jan 3, 2019

okay if i merge this and continue in a new PR to upgrade to php 7.1?

@Nyholm
Copy link
Member

Nyholm commented Jan 3, 2019

Yes, go ahead.

@Nyholm Nyholm mentioned this pull request Jan 3, 2019
@Nyholm
Copy link
Member

Nyholm commented Jan 3, 2019

I added #148 about dropping PHP 7.0

@dbu dbu merged commit 6a2df0d into 2.x Jan 3, 2019
@dbu dbu deleted the more-typing branch January 3, 2019 12:48
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.

4 participants