Api Changes for PHP Services #30
Conversation
…. Chullo now thinly wraps the API class. Chullo behaviour updated to return true/false in a lot of write scenarios.
Will have to check this, not sure if 200 is the only valid one, 304 is also valid if resource was not changed.
Not sure if `null` is good for the client or even 200 is the only response code we care 304 Not Modified is an option also which will also never return a body.
Basically check if the $mock and $result are the same since we are returning everything
Since i changed the response to getResource graph depends on the body. I hope it's good this time.
now checks if body and headers are the same. makes little sense since getResource is practically a pipe.
|
shame on me. We will have to discuss this pull anyway. If needed we can close it and start again based on what @daniel-dgi did. For some reason(looking at my keyboard) the merge upstream (daniel) had conflicts and the resolve i did not maintained attribution to dani's last work 😞 |
| return null; | ||
| } | ||
|
|
||
| return $response->getBody(); | ||
| return $response; |
There was a problem hiding this comment.
We should return the body here.
There was a problem hiding this comment.
Hi, i feel having the whole response is better. e.g, One need is to be able to now what mime type i'm getting back when getting a binary resource.
There was a problem hiding this comment.
Then use the FedoraApi class? Isn't that why we made it?
There was a problem hiding this comment.
You are probably right (100% right), but maybe it could be also an option to add a specific method for getting only the body() instead of having the same method name returning different things (like a getResourceContent in the FedoraClient Interface?). Just an idea, i'm sure you have good enough reasons. It's just i like the idea of obscuring a few errors when returning for the services but preserve headers, etc in case of success. Just an idea 😄 , since we really only need the clean body for https://github.com/Islandora-CLAW/chullo/pull/30/files#diff-9a4a06deeec43205acd722cefedf7f05R141
There was a problem hiding this comment.
Ha, yeah. So what you're referring to there is my horrible abuse of PHP interfaces. They don't actually take return type into consideration. So I just extended IFedoraClient from IFedoraApi and just noted the different return types in the comments. I'm totally aware of how that's probably misleading. I guess just make a completely seperate interface for IFedoraClient, then? I did purposefully like keeping the names the same since we're really just wrapping the Api with the Client.
As for needing mimetype (or other things), I figured you'd most likely know before hand when making the GET request. If you didn't know what the resource was, you could either use a HEAD request to find out, or hit the fcr:metadata resource in order to get the technical details on the file. Perhaps having a function that would return a simple struct, with both the body content and the fcr:metadata as an Easy_RdfGraph would be useful?
|
@DiegoPino any movement on this over the holidays? Let me know whenever you've got something and I'll give it a look over. |
Rollback on Chullo class implementation of getResource method. Returning only body again.
|
@daniel-dgi, reverted to return body on chullo's get resource implementation, i think that way we are back to the previous functionality. Test kept because they are over fedoraAPI. will handle the other stuff ASAP. Most of my advances are on this side, would love to have your comments. DiegoPino/islandora@24513d7 Still lots todo but advancing. Have to check/recheck edge cases, uuid generator (still thinking on passing that one to chullo instead of having it locally as a service provider in islandora services, etc). Will be out until thursday (missing claw call this time 😞 but working every day a bit. Happy new year and thanks for all your help during 2015! |
Changed chullo one back to only body Should we do test also for every corresponding API method?
When using the api from silex (not tested some where else), all header keys become lowercase. If the caller already has content-type in the headers (like a normal rest api call according to fedora4 documentation), then we get two "content-type" and "Content-Type" which makes guzzle fail with a "HTTP/1.1 400 Bad Request". It's silly but i tested this several times using different combinations for 'Content-Type' when calling via CURL. We should test other headers too maybe?
|
Ok, clearly trying to point to chullo from CLAW on the filesystem of my machine was the problem. Pointed to @DiegoPino's github and all is well. Merging. |
|
👏 |
|
😌 |
|
👏 👍 |
Includes @daniel-dgi updates and refactoring + some minor bug fixes. Will probably need some talk about responses and some tests on the resource methods.