Async and CallbackURL Added to API Wrapper

[rsands2801]

Ability to call setAsync() and also setCallbackUrl() for async requests.

I have left file_put_contents ability for async true as it is users decision to save to file or alternatively read the response.

[mediocretes]

It seems like the only way to get the async result in this implementation is to use the callback url. Because of that, perhaps it should just be one function like setAsyncWithCallback("your://url.here"), since every call with setAsync will also need a callback. That, and/or implement a second function that checks for the url and returns the document or null/false if it isn't ready (or which just blocks until it is ready, and polls at regular intervals).

[rsands2801]

Not neccessarily, with async true it can return a json reponse. It would then be up to the user to handle the json response as need to e.g. it could be a callback or it could be return json -> save to database and then they use a cron to ping /status/

But we could enhance wrapper to add on a checkStatus() if this would work?

[janxious]

Strongly in favor of following an API change similar to what is mentioned in this issue.

requestDocumentAsync could be a convenience function that just sets async and does a call to fetchDocument along with some response cleanup.

[rsands2801]

I am happy to code it into a sepertae function instead of fetchDocument()

To confirm thoughts are:

• a second function requestDocumentAsync() that can take in a callback URL as a parameter
• if requestDocumentAsync() callback given then set that
• checkStatus(ID) function which requires the ID in the function call
• fetchAsyncDocument(PATH) which requires the path to write to

That the general feeling of best way to go?

[rsands2801]

@janxious Is this what your team think is best?

https://github.com/rsands2801/php-docraptor/blob/requestDocumentAsync/src/ApiWrapper.php

[janxious]

A couple things:

• There is a typo at https://github.com/rsands2801/php-docraptor/blob/requestDocumentAsync/src/ApiWrapper.php#L263 (resonse -> response)
• I think I could make a case for doing some url format checking in there on the callback url

Like the direction it is headed, though.

---

When we add status/download to the wrapper, will need to modify the http client to do GETs, and probably do some refactoring. :smiley_cat:

[rsands2801]

@janxious A basic checkStatus() added for now, a doGet in httpclient for calling URI only

https://github.com/rsands2801/php-docraptor/tree/requestDocumentAsync/src

[janxious]

There's some kind of weird stuff in there. It's hard to comment on since it's not in PR. Basically:

• readme needs to be updated
• probably would restructure the http client doPost/doGet to be much simplified and pull out the copied code between them
• This line probably should not raise an exception.
• This method probably should not have a default value, but if it does, then it should be null.
• This switch could probably just check for the status in a set of things the wrapper knows about, and return it if found, otherwise throw the exception it is. This is more thinking along the not raising an exception for failed docs.

My untested re-write of HTTPClient's doPost looks like:

    /**
     * @param string $uri
     * @param array $postFields
     * @return mixed
     * @throws Exception
     */
    public function doPost($uri, array $postFields)
    {
        $curl_session = getCurlSession($uri);
        $queryString = http_build_query($postFields);
        curl_setopt($curl_session, CURLOPT_POST, count($postFields));
        curl_setopt($curl_session, CURLOPT_POSTFIELDS, $queryString);
        return doRequest($curl_session);
    }

    private function getCurlSession($uri) {
        $ch = curl_init();
        curl_setopt($ch, CURLOPT_URL, $uri);
        curl_setopt($ch, CURLOPT_RETURNTRANSFER, true);
        if ($this->config->getReportUserAgent()) {
            curl_setopt($ch, CURLOPT_USERAGENT, $this->userAgent());
        }
        return $ch;
    }

    private function doRequest($curl_session) {
        $result = curl_exec($curl_session);

        if (!$result) {
            throw new Exception(sprintf('Curl error: %s', curl_error($curl_session)));
        }

        $httpStatusCode = curl_getinfo($curl_session, CURLINFO_HTTP_CODE);

        if ($httpStatusCode != 200) {
            switch ($httpStatusCode) {
                case 400:
                    throw new BadRequestException();
                case 401:
                    throw new UnauthorizedException();
                case 403:
                    throw new ForbiddenException();
                case 422:
                    throw new UnprocessableEntityException();
                default:
                    throw new UnexpectedValueException($httpStatusCode);
            }
        }

        curl_close($curl_session);

        return $result;
    }

EDIT: renamed makeRequest-> doRequest

[rsands2801]

Yes there is things in there far from complete, how it handles responses, etc.

unsure of calling it doRequest as a request can be either a get request or a post request so it makes the doPost seem like a bolt on. Really doPost() can be removed completely and make doRequest(URI, $fields = null) - if $fields are given then its a post request naturally and it can set the post fields.

Switch needs changed as you mention, its a quick draft to get the basic functions working. I think its best checkStatus() should return the raw json response and let the user proceed however they wish. I dont think checkStatus() should do anything but return what your server says? Its a bit like many payment gateways, they return a json back and let the user handle it however they wish.

I think fetchDocumentAsync() should call checkStatus() and check status is complete from json and if so proceed to download contents.

Thoughts on the above?

[janxious]

doRequest is private, and just matches the verbiage we're using elsewhere. I think it is valuable to maintain the convenience methods of doPost/doGet that can be used outside of the HTTPClient class, so I don't have to go read that class to use it elsewhere.

I'm totally okay with raw JSON. Here's a workflow I can imagine.

/* checkStatus can handle the JSON payload from requestDocAsync or take an id */
$json_initial = $client->requestDocumentAsync();
/* the following two return the same thing */
$json_status = $client->checkStatus($json_initial);
$json_status = $client->checkStatus(json_decode($json_initial)->status_id);

/* fetchDocumentAsync can handle the JSON payload from requestDocAsync, checkStatus, or a
   download URL. Assuming the document completed properly, this returns file contents. There's a 
   case here where if the document failed and you try to call fetchDocumentAsync, we should 
   probably throw an exception. */
$file_content = $client->fetchDocumentAsync($json_initial);
$file_content = $client->fetchDocumentAsync($json_status);
$file_content = $client->fetchDocumentAsync(json_decode($json_status)->download_url);

I appreciate the last thoughts you gave me. Raw JSON is the way, but also we should make this super easy to use, so supporting multiple usages whilst allowing debugging is what I'm going for here.

[rsands2801]

I am not sure if I am time to fix this all up as it is turning into a rewrite of the calls and really setAsync() and set AsyncCallback() works easily. It would be like creating a whole new function for setTest()

I also think passing in a json payload is overkill, any developer will know to supply a proper value in.

fetchDocumentAsync should take in the status ID only, check status if its complete get the file and return. Also fetchDocumentAsync should be given a path to put the file to, we had to end using copy() to get the server to get these huge PDF files.

If someone wants to rewrite the httpclient I am happy to assist and rewrite the apiwrapper

[janxious]

I am not trying to put this major re-work on you. I am trying to come to an agreement on motion to take next. i.e. I am planning on doing the technical work for a lot of this.

---

I know this is a delay for your team, so for now, I'm gonna merge this, make a few adjustments, and bump version. Is that reasonable to you?

[rsands2801]

@janxious that would be great and will provide a means for users to do an async call - would be great as we are manually over riding vendor after a composer update at present

[janxious]

New version 1.3.0 released @rsands2801.