Skip to content
This repository was archived by the owner on Jan 8, 2024. It is now read-only.

Upgrade to CakePHP 3.4 #6

Merged
merged 7 commits into from
Aug 5, 2017
Merged

Upgrade to CakePHP 3.4 #6

merged 7 commits into from
Aug 5, 2017

Conversation

ADmad
Copy link
Contributor

@ADmad ADmad commented Aug 4, 2017

Q A
Bug fix? no
New feature? no
BC breaks? maybe
Deprecations? no
Related tickets n/a
Documentation n/a
License MIT

What's in this PR?

CakePHP dependency has been updated to 3.4

Why?

CakePHP 3.4 supports PSR7 so lesser code is required by this adapter. Easier maintenance :)

Checklist

  • Updated CHANGELOG.md to describe BC breaks / deprecations | new feature | bugfix
  • Documentation pull request created (if not simply a bugfix)

$cakeRequest->url((string) $request->getUri());
$cakeRequest->version($request->getProtocolVersion());
$cakeRequest->body($request->getBody()->getContents());
$cakeRequest = new Request(
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to create a new Request if CachePHP 3.4 supports PHP7?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes unfortunately Cake's HTTP client's method argument is typehinted as the concrete class Cake\Http\Client\Request instead of PSR7's interface. So for the time being this is required.

Copy link
Member

Choose a reason for hiding this comment

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

Hm, okej. Thanks.

@Nyholm
Copy link
Member

Nyholm commented Aug 4, 2017

Thank you. I think this is a good idea. Just make sure the tests are green and I'll be happy to review and merge.

composer.json Outdated
@@ -14,7 +14,7 @@
"php": "^5.5 || ^7.0",
"php-http/httplug": "^1.0",
"php-http/discovery": "^1.0",
"cakephp/cakephp": "^3.1"
"cakephp/cakephp": "^3.4"
Copy link
Member

Choose a reason for hiding this comment

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

Use 3.4.5 and the test will pass.

Versions before 3.4.5 had wrong dependencies.

@Nyholm
Copy link
Member

Nyholm commented Aug 4, 2017

hm, more errors. Try bumping the versions even more then =)

@ADmad
Copy link
Contributor Author

ADmad commented Aug 4, 2017

🎉

Copy link
Member

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

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

Awesome. There is just a code style fix.
I think this looks good and I will merge this one if not @joelwurtz disagrees

src/Client.php Outdated

if (null === $cakeRequest->header('Content-Type')) {
if ($cakeRequest->header('Content-Type') === null) {
Copy link
Member

Choose a reason for hiding this comment

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

We should use "yoda" syntax. Please revert this change.

@Nyholm
Copy link
Member

Nyholm commented Aug 4, 2017

Also. It would be great if you added a comment in the changelog for 0.2.0

@ADmad
Copy link
Contributor Author

ADmad commented Aug 4, 2017

I'll leave it to use guys to add appropriate info to changelog 😄

@ADmad
Copy link
Contributor Author

ADmad commented Aug 4, 2017

Copy link
Member

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

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

Thank you

@Nyholm Nyholm merged commit 5c25bc8 into php-http:master Aug 5, 2017
@ADmad ADmad deleted the cake-3.4 branch February 19, 2018 14:20
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants