Release 2.0.1 : fix post/put requests

* fix: POST/PUT requests handling * use correct content-type `x-www-form-urlencoded`or `multipart/form-data` when files supplied * refacto: GET/DELETE requests handling * remove: PATCH request (no used on gitlab api currently) * improvement: response error handling * test: update test * build: add missing extension ext-json to composer * ci: add php 7.1 and 7.2 environments on travis CI
emri99 · Dec 3, 2019 · c25a394 · c25a394
1 parent bc6c9c5
commit c25a394
Show file tree

Hide file tree

Showing 4 changed files with 79 additions and 57 deletions.
diff --git a/.travis.yml b/.travis.yml
@@ -1,10 +1,14 @@
 language: php
 
+dist: trusty
+
 php:
   - 5.4
   - 5.5
   - 5.6
   - 7.0
+  - 7.1
+  - 7.2
 
 before_script:
   - travis_retry composer self-update

diff --git a/composer.json b/composer.json
@@ -13,7 +13,8 @@
     ],
     "require": {
         "php": ">=5.4",
-        "mashape/unirest-php": "^3.0"
+        "mashape/unirest-php": "^3.0",
+        "ext-json": "*"
     },
     "autoload": {
         "psr-4": {

diff --git a/src/GitlabApiClient.php b/src/GitlabApiClient.php
@@ -172,7 +172,7 @@ public function authenticate($token, $authMethod = self::AUTH_HTTP_TOKEN, $sudo
      */
     public function get(array $parameters = array(), $headers = array())
     {
-        $response = $this->request(Method::GET, $this->flushPath(), $parameters, $headers);
+        $response = $this->request(Method::GET, $this->flushPath($parameters), $headers);
 
         return $this->returnOrThrow($response);
     }
@@ -186,33 +186,39 @@ public function get(array $parameters = array(), $headers = array())
      */
     public function post(array $parameters = array(), $headers = array(), array $files = array())
     {
-        $response = $this->request(Method::POST, $this->flushPath(), $parameters, $headers, $files);
+        $body = null;
+        if (!empty($parameters) && empty($files)) {
+            $headers['Content-Type'] = 'application/x-www-form-urlencoded';
+            $body = Request\Body::Form($parameters);
+        } elseif (!empty($files)) {
+            $headers['Content-Type'] = 'multipart/form-data';
+            $body = Request\Body::Multipart($parameters, $files);
+        }
 
-        return $this->returnOrThrow($response);
-    }
-
-    /**
-     * @param array $parameters
-     * @param array $headers
-     *
-     * @return mixed
-     */
-    public function patch(array $parameters = array(), $headers = array())
-    {
-        $response = $this->request(Method::PATCH, $this->flushPath(), $parameters, $headers);
+        $response = $this->request(Method::POST, $this->flushPath(), $headers, $body);
 
         return $this->returnOrThrow($response);
     }
 
     /**
      * @param array $parameters
      * @param array $headers
+     * @param array $files
      *
      * @return mixed
      */
-    public function put(array $parameters = array(), $headers = array())
+    public function put(array $parameters = array(), $headers = array(), array $files = array())
     {
-        $response = $this->request(Method::PUT, $this->flushPath(), $parameters, $headers);
+        $body = null;
+        if (!empty($parameters) && empty($files)) {
+            $headers['Content-Type'] = 'application/x-www-form-urlencoded';
+            $body = Request\Body::Form($parameters);
+        } elseif (!empty($files)) {
+            $headers['Content-Type'] = 'multipart/form-data';
+            $body = Request\Body::Multipart($parameters, $files);
+        }
+
+        $response = $this->request(Method::PUT, $this->flushPath(), $headers, $body);
 
         return $this->returnOrThrow($response);
     }
@@ -225,78 +231,77 @@ public function put(array $parameters = array(), $headers = array())
      */
     public function delete(array $parameters = array(), $headers = array())
     {
-        $response = $this->request('DELETE', $this->flushPath(), $parameters, $headers);
+        $response = $this->request('DELETE', $this->flushPath($parameters), $headers);
 
         return $this->returnOrThrow($response);
     }
 
     /**
      * @param Response $response
      *
-     * @return array|\stdClass depending on option 'json_decode_to_array'
-     *
      * @throws \RuntimeException
+     *
+     * @return array|\stdClass depending on option 'json_decode_to_array'
      */
     protected function returnOrThrow($response)
     {
         if (!$response) {
             throw new \RuntimeException('No response');
         }
 
-        if (null === $response->body) {
-            throw new \RuntimeException('Unable to decode response');
+        if (null === $response->raw_body) {
+            return null;
         }
 
-        $exception = null;
-        if ($this->options['json_decode_to_array']) {
-            $exception = @$response->body['exception'];
-        } elseif (is_object($response->body)) {
-            $exception = @$response->body->exception;
+        if (is_string($response->body) && $response->headers["Content-Type"] === "application/json") {
+            throw new \RuntimeException('Unable to decode json response');
         }
 
-        if ($exception || $response->code >= 400) {
-            throw new \RuntimeException(sprintf('%s - %s', $response->code, @$response->body->message));
+        $error = !empty($response->body->error) ? $response->body->error : null;
+        if ($error && !is_string($error)) {
+            $error = trim(json_encode($error), '"');
         }
 
-        return $response->body;
+        if ($response->code >= 400) {
+            $error = $error ?: '';
+            if (404 === $response->code && $this->lastpath) {
+                $error .= $error.' (url : '.$this->lastpath.')';
+                $this->lastpath = null;
+            }
+        }
+
+        if ($error !== null) {
+            throw new \RuntimeException(sprintf('%s - %s', $response->code, $error), $response->code);
+        }
+
+        if (!$this->options['json_decode_to_array']) {
+            return $response->body;
+        }
+
+        return json_decode(json_encode($response->body), true);
     }
 
-    protected function request($httpMethod = 'GET', $path, array $parameters = array(), array $headers = array(), array $files = array())
+    protected function request($httpMethod = 'GET', $path, array $headers = array(), $body = null)
     {
         $httpMethod = strtoupper($httpMethod);
         $path = trim($this->baseUrl.$path, '/');
-        $body = empty($files) ? $parameters : Request\Body::Multipart($parameters, $files);
 
         $httpOptions = array(
             'verifyPeer' => array($this->options['verify_peer']),
             'verifyHost' => array($this->options['verify_host']),
             'timeout' => array($this->options['timeout']),
-            'jsonOpts' => array($this->options['json_decode_to_array'], 512, $this->options['json_decode_options']),
+            'jsonOpts' => array(false, 512, $this->options['json_decode_options']),
         );
 
         $allHeaders = array_merge($this->headers, $this->authenticationHeaders, $headers);
 
-        $response = $this->performRequest(
+        return $this->performRequest(
             $httpMethod,
             $path,
             $body,
             $allHeaders,
             $httpOptions
         );
-
-        if (is_object($response) && $response->code === 404) {
-            $message = @$response->body->message ?: '';
-            $message .= sprintf(' (url : %s)', $path);
-            if (is_array($response->body)) {
-                $response->body['message'] = $message;
-            } elseif (is_object($response->body)) {
-                $response->body->message = $message;
-            } else {
-                $response->body = json_encode(array('message' => $message), JSON_UNESCAPED_SLASHES);
-            }
-        }
-
-        return $response;
     }
 
     /**
@@ -308,6 +313,8 @@ protected function request($httpMethod = 'GET', $path, array $parameters = array
      * @param array  $allHeaders
      * @param array  $httpOptions
      *
+     * @throws \Unirest\Exception
+     *
      * @return \Unirest\Response
      */
     protected function performRequest($httpMethod, $path, $body, $allHeaders, $httpOptions)
@@ -334,11 +341,21 @@ protected function encodePath($path)
     /**
      * Retrieve current url fragment & flush it from next requests.
      *
+     * @param null|mixed $parameters
+     *
      * @return string
      */
-    protected function flushPath()
+    protected function flushPath($parameters = null)
     {
         $path = $this->path;
+
+        if ($parameters) {
+            $separator = false === strpos($path, '?') ? '?' : '&';
+            $path .= $separator.http_build_query($parameters);
+        }
+
+        // remember full url to display path in 404 message
+        $this->lastpath = $this->baseUrl.$path;
         $this->path = '';
 
         return $path;

diff --git a/tests/GitlabApiClientTest.php b/tests/GitlabApiClientTest.php
@@ -35,8 +35,8 @@ public function testBuildComplexPath()
             ->method('performRequest')
                 ->with(
                     'GET',
-                    self::BASE_PATH.'/path/1/to/data%20one/for/test',
-                    array('param2' => 'value2'),
+                    self::BASE_PATH.'/path/1/to/data%20one/for/test?param2=value2',
+                    null,
                     $this->isType('array'),
                     $this->isType('array')
                 )
@@ -131,8 +131,8 @@ public function testAuthenticateOauthToken()
         $sut->expects($this->once())
             ->method('performRequest')
             ->with(
-                'PATCH',
-                self::BASE_PATH.'/path/to/patch',
+                'PUT',
+                self::BASE_PATH.'/path/to/put',
                 $this->anything(),
                 $this->callback(function ($datas) use ($self) {
                     $self->assertEquals(array('Authorization' => 'Bearer my-precious',
@@ -146,8 +146,8 @@ public function testAuthenticateOauthToken()
 
         $sut
             ->authenticate($token, SUT::AUTH_OAUTH_TOKEN)
-            ->path('to', 'patch')
-            ->patch();
+            ->path('to', 'put')
+            ->put();
 
         $sut = $this->getMockedSUT();
         $sut->expects($this->once())
@@ -220,7 +220,7 @@ public function testNoResponse()
     }
 
     /**
-     * @expectedExceptionMessage Unable to decode response
+     * @expectedExceptionMessage Unable to decode json response
      * @expectedException \RunTimeException
      */
     public function testBadResponse()
@@ -236,7 +236,7 @@ public function testBadResponse()
                 $this->anything(),
                 $this->anything()
             )
-            ->willReturn(new Response(200, null, '', array()));
+            ->willReturn(new Response(200, '{malformed json}', 'Content-Type: application/json', array()));
 
         $sut->path()->get();
     }