opsway / zohobooks-api

ZohoBooks PHP library
MIT License
6 stars 10 forks source link

Update `Client` in order to use request body instead of query string with `post()` and `put()` methods #23

Closed phansys closed 7 years ago

phansys commented 7 years ago
Q A
Branch master
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets n/a
License MIT
Doc PR n/a

These changes prevents errors like "zoho Internal API error: 414 Request-URI Too Large", since the whole request was encoded and sent as query string.

Also, the way in which the array concat was done (+ operator order) in Client methods is updated in order to use Client::getParams() as a source for default values, but allowing them to be overridden by the method's arguments.

The call to \json_encode() was replaced by \GuzzleHttp\json_encode() at Client::getParams(), in order to prevent this kind of errors with URI sizes.

codecov[bot] commented 7 years ago

Codecov Report

Merging #23 into master will decrease coverage by 0.06%. The diff coverage is 0%.

Impacted file tree graph

@@             Coverage Diff             @@
##             master     #23      +/-   ##
===========================================
- Coverage      6.82%   6.76%   -0.07%     
  Complexity      112     112              
===========================================
  Files            12      12              
  Lines           205     207       +2     
===========================================
  Hits             14      14              
- Misses          191     193       +2
Impacted Files Coverage Δ Complexity Δ
src/Client.php 0% <0%> (ø) 17 <0> (ø) :arrow_down:

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 f8ae3ba...b86b036. Read the comment docs.

mrVrAlex commented 7 years ago

Maybe have sense write unit tests for getList(), post(), put(), get() methods with httpClient mock in this PR?

phansys commented 7 years ago

@shandyDev, since these changes are related on how the arguments are provided to the base client (Guzzle), I don't know how to add tests to this library, since these arguments aren't handled by the library itself. Have you any thought about this?

mrVrAlex commented 7 years ago

Ok I merged it. Unit test I will try write later on this week, separatly.