stevenmaguire / trello-php

A php client for consuming the Trello API
MIT License
87 stars 25 forks source link

Custom Fields API? #27

Closed andykirk closed 6 years ago

andykirk commented 6 years ago

Hi,

Trello recently launched the custom fields API and I was wondering if there were plans to include it in this codebase? I see it hasn't been touched in a year so I'm not sure if it's actively maintained, but this code is SO super useful and seems really well written, so I figured it was worth asking,

Many thanks, Andy

stevenmaguire commented 6 years ago

I'll def look at it! The package hasn't changed much because I haven't seen many changes lately. It is def actively maintained. Thanks for bringing by this new functionality to my attention.

andykirk commented 6 years ago

Great! I though put this repo probably wasn’t abandoned but I wanted to check. Really glad to hear you’ll be taking a look. I’ve had a tinker with the API via curl, but I’m not sure really getting anywhere adding custom field values to a new card. Some of the docs aren’t all that clear (to me). Happy to help with any testing or whatever.

Cheers.

stevenmaguire commented 6 years ago

I've added new functionality, tests, and documentation to support the custom fields resources documented on Trello's site. https://developers.trello.com/docs/getting-started-custom-fields

It is now available in the 0.5.0 tag.

https://github.com/stevenmaguire/trello-php/releases/tag/0.5.0

andykirk commented 6 years ago

Wow, that was super fast! Thanks so much.

I'll give it whirl.

andykirk commented 6 years ago

Hi, I'm just tinkering around with this and there are a couple of things (so far) that are either missing or I don't understand: Firstly, nested routes for getting custom fields for cards:(https://developers.trello.com/v1.0/reference#customfieldsidoptionsidcustomfieldoption-1)

/1/cards/{idCard}/customField/{idCustomField}/item

-- I can't find this route in the list of methods in ApiMethodsTrait.php.

Secondly, I think the signature of the getCard method needs updating to allow custom fields to be included in that call as per https://developers.trello.com/v1.0/docs/getting-started-custom-fields#section-getting-customfielditems-for-cards

"Get CustomFieldItem For Single Card We can also get the customFieldItem for a single card by making a GET request to the /1/cards/{cardId} endpoint and passing in the parameter customFieldItems=true:"

curl -X GET -H "Content-Type: application/json" \
https://api.trello.com/1/cards/5a4d294ff7239936994177f3/?fields=name&customFieldItems=true/

Something like:

$result = $client->getCard($cardId, $includeCustomFieldsBool);

Or maybe a separate

$result = $client->getCardCustomFields($cardId);

and have the extra query params part of the definition?

Any thoughts? Please let me know if I'm just being an idiot.

Cheers.

andykirk commented 6 years ago

Update - I found that just passing those values as parameters works:

$result = $client->getCard($cardId, array('fields'=>'name', 'customFieldItems'=>'true'));

In case you want to just add that to the docs without creating an extra method?

Cheers.

stevenmaguire commented 6 years ago

I'll add the missing nested resource shortly.

You are right to use the attributes collection to set all query string params for API calls. That is a universal pattern in the library. If you feel that is not clear, feel free to update the documentation to make that clearer. As a tip, I think some high-level documentation (in the README) would be a good first step rather than explicitly listing the document on each end point (in the API-GUIDE) since the pattern is package wide. I'd love help with documentation :)

stevenmaguire commented 6 years ago

I've pushed up some new changes to cover nested custom fields for specific cards. It seems that the getter was missing. The set/update method was already in place. They are listed in the API-GUIDE.md now.

# Get card custom field - just added
$result = $client->getCardCustomField($cardId, $customFieldId);

# Update card custom field - already implemented
$result = $client->updateCardCustomField($cardId, $customFieldId, $attributes);

Those changes are now available on tag 0.5.1

You can review the changes here https://github.com/stevenmaguire/trello-php/commit/77a85b653a388f18f76e64871e7f3801c5b978f4

andykirk commented 6 years ago

Hi, Happy to try and provide some help with the docs - it's the least I can do,

In the meantime - I think there's still a problem with updateCardCustomField. From what I can tell, the 'value' needs to be in the payload of the PULL rather than the URL, but from what I can tell from the code, that isn't happening. I'm not sure if this is an exception to the norm or not. Basically, I couldn't get anything to update a cards custom field value (I tried cURL, JS, the Try It tool etc.) so I found this thread: https://community.atlassian.com/t5/Trello-questions/How-do-I-set-the-value-of-a-Trello-card-s-custom-field-using-the/qaq-p/757887 I eventually got it working in JS using this:

var xhr = new XMLHttpRequest();

xhr.addEventListener("readystatechange", function () {
  if (this.readyState === this.DONE) {
    console.log(this.responseText);
  }
});

xhr.open('PUT', 'https://api.trello.com/1/card/' + CARD_ID + '/customField/' + FIELD_ID + '/item?key=' + TRELLO_API_KEY + '&token=' + TRELLO_API_TOKEN, true);
xhr.setRequestHeader('Content-type','application/json; charset=utf-8');
xhr.send(JSON.stringify({"value": {"number": "3"}}));

Screenshot from devtools if that helps: image

I don't think there's a way to do this via the URL, so I'm not sure how much of a pain it'll be to get this to work? Let me know if there's anything I can to help with this somehow.

Thanks, Andy

stevenmaguire commented 6 years ago

@andykirk I don't really understand your issue or your question, and I am not sure what you mean by "the 'value' needs to be in the payload of the PULL rather than the URL".

The following code should work, as it is a direct translation of what you've posted via JS:

$cardId = 'CARD_ID';
$customFieldId = 'FIELD_ID';
$attributes = [
    "value" => [
        "number" =>  3 
    ]
];
$result = $client->updateCardCustomField($cardId, $customFieldId, $attributes);

If this does not work, please post the errors you're encountering from this library.

stevenmaguire commented 6 years ago

@andykirk I think I found the issue and it may be legit. It seems that the initial updateCardCustomField method was implemented with the wrong path:

Was: cards/%s/customField/%s Now: cards/%s/customField/%s/item

Take a look at tag 0.5.2 for this fix. https://github.com/stevenmaguire/trello-php/releases/tag/0.5.2

I am guessing the issue that you encountered was a 404 (not found) or 400 (bad request) or something similar. The documentation does suggest the latter to be correct. I do still think that, with this fix in place, the pattern mentioned in my previous response should work correctly. Please pull the latest version and give it a try. If it does not, please share with me error text from this library, not working code from other languages or curl; I need to understand what is going wrong within this library.

andykirk commented 6 years ago

Hi, sorry if I was vague. I’d been wrestling with trying to update a custom field value for a card on and off all day and my brain was mush. I’ll give the latest one a go and report back.

Thanks.

andykirk commented 6 years ago

Hi again, So I updated to 0.5.2 and I get the same problem. I'll attempt to explain:

This PHP snippet:

$attributes = array(
    "value" => array(
        "number" =>  "3" // or int - same result
      )
);
$result = $client->updateCardCustomField($cardId, $customFieldId, $attributes);

Gives this error:

Fatal error: Uncaught exception 'GuzzleHttp\Exception\ClientException' with message 'Client error: `PUT https://trello.com/1/cards/5ab3afbf05fda5a0172f0a4f/customField/5a986717d6afbd6de1d24e18/item?value%5Bnumber%5D=3&key=<DELETED>&token=<DELETED>` resulted in a `400 Bad Request` response: Invalid custom field item value. ' in <DELETED>/vendor/guzzlehttp/guzzle/src/Exception/RequestException.php:113 Stack trace: #0 <DELETED>/vendor/guzzlehttp/guzzle/src/Middleware.php(66): GuzzleHttp\Exception\RequestException::create(Object(GuzzleHttp\Psr7\Request), Object(GuzzleHttp\Psr7\Response)) #1 <DELETED>/vendor/guzzlehttp/promises/src/Promise.php(203): GuzzleHttp\Middleware::GuzzleHttp\{closure}(Object(GuzzleHttp\Psr7\Response)) #2 <DELETED>/guzzlehttp/promises/src/Promise.php(156): GuzzleHttp\Promise\Promise::callHandler(1, Object(Guzz in <DELETED>/vendor/stevenmaguire/trello-php/src/Http.php on line 292

Note the 'value' object is a param in the URL.

Now, Trello's own Try It tool (https://developers.trello.com/v1.0/reference#customfielditemsid) generates a URL in the same way and constantly fails. However, the docs below show that the 'value' object needs to be sent as the body of the request (payload), not in the URL:

curl -X PUT -H "Content-Type: application/json" \
https://api.trello.com/1/card/5a300b5036c4681398d3dab1/customField/5a6a23abf958725e1ac86c21/item \
-d '{
  "value": "",
  "key": "",
  "token": ""
}'

E.g. image

Switching to the JS tab reveals this:

var data = null;

var xhr = new XMLHttpRequest();

xhr.addEventListener("readystatechange", function () {
  if (this.readyState === this.DONE) {
    console.log(this.responseText);
  }
});

xhr.open("PUT", "https://api.trello.com/1/card/<DELETED>/customField/<DELETED>/item?value=%7B%22number%22%3A%203%7D&key=<DELETED>&token=<DELETED>");

xhr.send(data);

try running this code from the console (with real data, of course) and it doesn't work - same errors. However, moving the 'value' object into the body of the request as I've put above does work. To my mind, this illustrates that passing the 'value' object via the URL is never going to work. The line: return call_user_func_array([$this->getHttp(), $signature[0]], $parameters); suggests to me that there isn't a mechanism for passing anything but URL parameters to the HTTP object, but after that I can't follow the code very easily. So, unless I've misunderstood something, I think this library needs some changes to allow for a body/payload in this PUT request and to send it as application/json. I've not found other API calls that fail in this way yet, so I'm not sure if this a unique exception.

If you contact me privately I can let you see the a live demo where this is happening, if you haven't already got something like that set up. If your tests show that it really does work as you expect, I'd be really grateful to see some working example.

Many thanks, Andy

stevenmaguire commented 6 years ago

@andykirk Thank you for the thorough documentation of what you're seeing. I believe I've located the issue and it's a real face palm moment, maybe half on my part and half on Trello's part. It seems that in most (all?) of the other PUT and POST operations all the data is required to be passed via query string, except this new one. I had forgotten this and was operating under the assumption that POST and PUT were passing as body. That is not the case. So, I've put in a fix for this one operation to handle the request as a PUT with body.

Can you pull the latest from master branch and give it a go locally? If it works out ok, I will cut a new release. Sorry for the confusion.

andykirk commented 6 years ago

Ok great, I’ll give it a go as soon as I can. I’m just relieved it’s not me going nuts 🙂

andykirk commented 6 years ago

Hi, It's NEARLY there. I used the latest code and got same error, but then I remembered it needs to be sent with the ['Content-type' => 'application/json'] header, and that's what was missing. I hacked my local copy of the code to include that header for putAsBody and it works! Huzzah!

If you like, I can raise a PR with my hack so you can see what I did, but I suspect you'll have a much better way of doing it. Let me know.

Cheers.

stevenmaguire commented 6 years ago

@andykirk I really appreciate your patience and cooperation in testing these latest changes. I have not been doing full integration tests with the API during this round of work as much as I should've been. So, you're doing the lion's share of that and I appreciate it. After I saw your last message I had an "OMG! Duh!" moment because that header is a very important piece of the pie - a piece that is usually sorted out much earlier when APIs expect JSON bodies on POST and PUT. Thanks for catching that. I think we're in good shape now. I pushed up another update to master. If you wouldn't mind giving it another go, I would appreciate it. If all is clear, I'll cut a new tag and we'll put this round of work down for a bit :)

andykirk commented 6 years ago

Ok great. No problem. I'll give this a go tomorrow but if that header's getting set now then it should be fine.

I'll let you know how I get on, and take a look at the docs next. I'm finding I'm looking for what I want to do in the Trello docs, then comparing the URL examples with the signatures in APITraits file, rather than using your API doc file, just so I can be sure I'm getting the right method call. I'm wondering if the API signatures shouldn't appear on the API doc file too, for clarity. That work certainly make it easier for me. Perhaps even a link cross referencing with the Trello docs? May be too much? I'll have a think.

Cheers.

stevenmaguire commented 6 years ago

I would certainly merge a PR that improved the documentation of the API-GUIDE.md in an intuitive and maintainable way (consistent pattern for resource path formats and links to trello docs?) if you felt up to it. the current level of offering is a minimal approach to help get folks started and leave some breadcrumbs. Referencing the ApiMethodsTrait is not a bed behavior - I think it's a great practice to actually get in and see how the code works if you're using a library - and I do think there's lots of room to improve on the non-code documentation.

andykirk commented 6 years ago

Hi, Sorry for the delay. I can confirm this works!

Thanks.

stevenmaguire commented 6 years ago

Awesome. 0.5.3 is available with these changes now.

andykirk commented 6 years ago

Perfect! Thanks for being so responsive. I've not forgotten about the docs, will get you something in the next few days.