Add support for asset creation #86

kasperg · 2013-11-13T10:20:03Z

Based on #85 I have taken a first stab at support for creating assets.

I do not have that much experience using this library or its code style so feedback would be much appreciated.

The related test case passes but I have tried to create an actual asset using the following code:

$client = new Github\Client();
$client->authenticate($username, $password, Github\Client::AUTH_HTTP_PASSWORD);
$return = $client->api('repo')->releases()->assets()->create('kasperg', 'release-asset-test', '92217', array('name' => 'test.txt', 'content-type' => 'text/plain', 'body' => '123'));

This fails with the following exception. I am unsure why.

Github\Exception\RuntimeException: You have reached GitHub hour limit! Actual limit is: 5000 in /Users/kasper/Code/php-github-api/lib/Github/HttpClient/HttpClient.php on line 138

The corresponding cURL request works fine:

curl --user "kasperg" -H "Content-Type: text/plain" --data-binary test.txt "https://uploads.github.com/repos/kasperg/release-asset-test/releases/92217/assets?name=test1.txt"

If we at some point decide to merge this, the documentation will have to be updated as well.

cursedcoder · 2013-11-15T11:43:58Z

lib/Github/Api/Repository/Assets.php

@@ -41,6 +43,52 @@ public function show($username, $repository, $id)
        return $this->get('repos/'.rawurlencode($username).'/'.rawurlencode($repository).'/releases/assets/'.rawurlencode($id));
    }

+


blank line should be removed

cursedcoder · 2013-11-15T11:47:35Z

Please use 4-spaces indentation for methods bodies instead of 2-spaces

cursedcoder · 2013-11-15T11:49:05Z

test/Github/Tests/Api/Repository/AssetsTest.php

@@ -43,6 +43,25 @@ public function shouldGetSingleReleaseAsset()
    /**
     * @test
     */
+    public function shouldCreateReleaseAsset()


https://travis-ci.org/KnpLabs/php-github-api/jobs/13906045 skip it for 5.3.3

kasperg · 2013-11-15T12:35:00Z

Thanks for the comments @cursedcoder. I have fixed the broken test and the code style issues.

I am still unable to actually upload an asset using the new method. Uploading with cURL works fine.

cursedcoder · 2013-11-15T13:30:00Z

@kasperg that because body is posted as json hash, you will need to define new method like postBinary

kasperg · 2013-11-15T13:50:32Z

Thank you for the suggestion. I will look into that.

When receiving a validation error (as in KnpLabs#86) there is no `X-RateLimit-Remaining` header included with the response. That is quite confusing. Here `$remaining` when cast to a string is set to "" (empty string). This does not strictly equal null and thus a `ApiLimitExceedException` is thrown. This change fixes this behavior. cURL requests/responses: ``` # Request: POST /repos/kasperg/release-asset-test/releases/92217/assets?name=test2.txt HTTP/1.1 Host: uploads.github.com 0: Accept: application/vnd.github.v3+json 1: User-Agent: php-github-api (http://github.com/KnpLabs/php-github-api) Content-Type: text/plain, text/plain User-Agent: Guzzle/3.7.4 curl/7.30.0 PHP/5.3.27 Authorization: Basic a2FzcGVyZzpwaW5rbW9ucw== Content-Length: 14 {"body":"122"} # Response: HTTP/1.1 422 status code 422 Cache-Control: no-cache Content-Length: 214 Content-Type: application/json; charset=utf-8 X-Content-Type-Options: nosniff X-Github-Media-Type: github.beta; format=json X-Github-Request-Id: a6633df9-4dff-11e3-800a-0be28f962e12 Date: Fri, 15 Nov 2013 14:10:15 GMT {"message":"Validation Failed","request_id":"a6633df9-4dff-11e3-800a-0be28f962e12","documentation_url":"http://developer.github.com/v3","errors":[{"resource":"ReleaseAsset","code":"already_exists","field":"name"}]} ```

cursedcoder · 2013-11-16T22:38:40Z

@kasperg please try this patch https://gist.github.com/cursedcoder/7506314

When receiving a validation error (as in KnpLabs#86) there is no `X-RateLimit-Remaining` header included with the response. That is quite confusing. Here `$remaining` when cast to a string is set to "" (empty string). This does not strictly equal null and thus a `ApiLimitExceedException` is thrown. This change fixes this behavior. cURL requests/responses: ``` # Request: POST /repos/kasperg/release-asset-test/releases/92217/assets?name=test2.txt HTTP/1.1 Host: uploads.github.com 0: Accept: application/vnd.github.v3+json 1: User-Agent: php-github-api (http://github.com/KnpLabs/php-github-api) Content-Type: text/plain, text/plain User-Agent: Guzzle/3.7.4 curl/7.30.0 PHP/5.3.27 Authorization: Basic a2FzcGVyZzpwaW5rbW9ucw== Content-Length: 14 {"body":"122"} # Response: HTTP/1.1 422 status code 422 Cache-Control: no-cache Content-Length: 214 Content-Type: application/json; charset=utf-8 X-Content-Type-Options: nosniff X-Github-Media-Type: github.beta; format=json X-Github-Request-Id: a6633df9-4dff-11e3-800a-0be28f962e12 Date: Fri, 15 Nov 2013 14:10:15 GMT {"message":"Validation Failed","request_id":"a6633df9-4dff-11e3-800a-0be28f962e12","documentation_url":"http://developer.github.com/v3","errors":[{"resource":"ReleaseAsset","code":"already_exists","field":"name"}]} ```

kasperg · 2013-11-17T12:21:33Z

@cursedcoder Thanks for the suggestion. It's a simple solution which should would work.

Before I saw this I started working on some a more serious refactoring which moves JSON encoding from Github\HttpClient\HttpClient to Github\Api\AbstractApi (a67737a).

Let me know what you think goes best with the structure of the project.

cursedcoder · 2013-11-17T15:13:01Z

lib/Github/Api/AbstractApi.php

+     * @param $parameters   Request parameters
+     * @return null|string
+     */
+    protected function createJsonBody(array $parameters) {


curly brace should be on next line

cursedcoder · 2013-11-17T15:14:24Z

@kasperg PR looks good

kasperg · 2013-11-17T17:33:30Z

Great! I'll clean up the commit history a bit and then it should be good to go.

… encoded parameters. This is needed to support Asset creation where the request body must be the content of the asset. This involves the following changes - Updated HttpClient methods to accept an untyped body instead of an array of parameters - Moved JSON encoding to AbstractApi and updated documentation accordingly - Updated test cases

Added coresponding unit test for PHP 5.3.4 or newer. SSL extensions are not fully working for 5.3.3. See travis-ci/travis-ci#1385. Updated documentation to show that Asset creation is now supported.

Add support for asset creation

cursedcoder · 2013-11-19T10:49:33Z

merged, thanks @kasperg

kasperg mentioned this pull request Nov 13, 2013

Release asset upload support #85

Closed

cursedcoder reviewed Nov 15, 2013
View reviewed changes

kasperg mentioned this pull request Nov 15, 2013

Do not report validation errors as rate limits #87

Merged

cursedcoder reviewed Nov 17, 2013
View reviewed changes

kasperg added 2 commits November 17, 2013 20:32

Add support for asset creation

bad663d

Added coresponding unit test for PHP 5.3.4 or newer. SSL extensions are not fully working for 5.3.3. See travis-ci/travis-ci#1385. Updated documentation to show that Asset creation is now supported.

cursedcoder added a commit that referenced this pull request Nov 19, 2013

Merge pull request #86 from kasperg/asset-create

2ff0101

Add support for asset creation

cursedcoder merged commit 2ff0101 into KnpLabs:master Nov 19, 2013

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Add support for asset creation #86

Add support for asset creation #86

Uh oh!

kasperg commented Nov 13, 2013

Uh oh!

cursedcoder Nov 15, 2013

Uh oh!

cursedcoder commented Nov 15, 2013

Uh oh!

cursedcoder Nov 15, 2013

Uh oh!

kasperg commented Nov 15, 2013

Uh oh!

cursedcoder commented Nov 15, 2013

Uh oh!

kasperg commented Nov 15, 2013

Uh oh!

cursedcoder commented Nov 16, 2013

Uh oh!

kasperg commented Nov 17, 2013

Uh oh!

cursedcoder Nov 17, 2013

Uh oh!

cursedcoder commented Nov 17, 2013

Uh oh!

kasperg commented Nov 17, 2013

Uh oh!

cursedcoder commented Nov 19, 2013

Uh oh!

Uh oh!

		@@ -41,6 +43,52 @@ public function show($username, $repository, $id)
		return $this->get('repos/'.rawurlencode($username).'/'.rawurlencode($repository).'/releases/assets/'.rawurlencode($id));
		}

Uh oh!

Add support for asset creation #86

Add support for asset creation #86

Uh oh!

Conversation

kasperg commented Nov 13, 2013

Uh oh!

cursedcoder Nov 15, 2013

Choose a reason for hiding this comment

Uh oh!

cursedcoder commented Nov 15, 2013

Uh oh!

cursedcoder Nov 15, 2013

Choose a reason for hiding this comment

Uh oh!

kasperg commented Nov 15, 2013

Uh oh!

cursedcoder commented Nov 15, 2013

Uh oh!

kasperg commented Nov 15, 2013

Uh oh!

cursedcoder commented Nov 16, 2013

Uh oh!

kasperg commented Nov 17, 2013

Uh oh!

cursedcoder Nov 17, 2013

Choose a reason for hiding this comment

Uh oh!

cursedcoder commented Nov 17, 2013

Uh oh!

kasperg commented Nov 17, 2013

Uh oh!

cursedcoder commented Nov 19, 2013

Uh oh!

Uh oh!