Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added a new file for Simple PHP interaction with Summon API #2

Closed
wants to merge 5 commits into from

Conversation

rushabhpasad
Copy link
Contributor

No description provided.

@rushabhpasad rushabhpasad reopened this Sep 4, 2013
@rushabhpasad
Copy link
Contributor Author

A simple example for PHP implementation:

<?php

$q = $_GET["query"];
if(isset($q)){
    $format = "json";

    header('Content-type: application/'.$format);
    $apiId = "<api id>";
    $apiKey = "< api key >";

    require_once dirname(__FILE__) . '/php_classes/PHP.php';

    $query = new SerialsSolutions_Summon_Query($q);
    $summon = new SerialsSolutions_Summon_PHP($apiId, $apiKey, array('responseType' => $format));
    echo $summon->query($query,'search' ,'GET',true);
} else {
?>
<html>
    <head>
        <title>Summon Finder</title>
    </head>
    <body>
        <form>
            <input type="text" name="query" /> <input type="submit" value="Go" />
        </form>
    </body>
</html>
<?php } ?>

@demiankatz
Copy link
Contributor

My only concern with this is that it changes the signature of query() in such a way that backward-compatibility is broken. Would you object to keeping $returnErr as the second parameter and adding the new parameters to the end of the list so that existing code won't break?

@rushabhpasad
Copy link
Contributor Author

Agreed. I have no objections to that.

@demiankatz
Copy link
Contributor

Great, thanks -- I'll take a closer look at this in the next day or two and get it merged soon.

@rushabhpasad
Copy link
Contributor Author

Awesome. Appreciate your fast response. :)

@demiankatz
Copy link
Contributor

I have incorporated most of your changes into master, with a handful of changes:

1.) I renamed your PHP class to CURL to be more specific -- since there are several different possible PHP approaches to HTTP, it seemed best to specify that you are using CURL.

2.) I removed the CURL class constructor since it was not necessary.

3.) I did not add all of the new parameters to the query() method. Only $raw seemed like a useful addition, since you don't send queries to a different endpoint, and using POST instead of GET seems to cause an error. I also added a $raw parameter to getRecord() for further flexibility. Please let me know if there is a use case I am overlooking.

4.) I made a few minor style changes to comply with PEAR standards (which have been used on all of the other files in this project).

There is one small outstanding issue which may not be of great importance: your CURL class currently ignores the $method parameter of the httpRequest() method, so it always does a GET request. As far as I know, the Summon API does not currently work with POST requests, so this support may not be necessary -- but for future reference, it may be worth implementing it at some point.

@demiankatz demiankatz closed this Sep 5, 2013
@rushabhpasad
Copy link
Contributor Author

I am fine with #1,#2 and #4.

For #3, the variable '$service' seems useful to be parameterized as summon also supports other services like image and availability. A different handling may be required for different services; I will have a look at it in a few days.

Different handling is required for different operations, and since summon only supports GET, I didn't implement POST/PUT/DELETE. In the future, at any point, if Summon releases support for other requests, we can incorporate them.

@demiankatz
Copy link
Contributor

If you want to access a service other than search, it would be better to simply access call() directly. Since the query() routine includes query processing logic that is irrelevant to services like image and availability, it doesn't seem appropriate to use it for those lookups. Perhaps the best solution is to simply make call() a public rather than a protected method, or to create some more generic public wrapper around it.

@rushabhpasad
Copy link
Contributor Author

I can see two options at the moment.

1.) We make call() public.
2.) We implement different methods such as query_search(), query_image() etc. for different services.

I prefer option 2 as we can handle the services independently and leverage call() in those methods.
Also, a different class for each service query needs to be defined, similar to Query.php which is specific to the search service.

@demiankatz
Copy link
Contributor

Option 2 makes sense to me. Depending on the complexity of each call, it may or may not be necessary to define a class for input parameters -- in some cases it may be acceptable to simply use an array or a set of function parameters. In any case, if you want to build some methods that meet your needs and open new pull requests, I'll be happy to offer further suggestions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants