Right now I'm doing

if (variable_get('salsa_api_url', NULL) && 
    variable_get('salsa_api_username', NULL) && 
    variable_get('salsa_api_password', NULL)) {

to see if it's safe to attempt to connect to Salsa. This is not ideal, because:
1) Salsa itself could be down
2) These variables might be set, but salsa_api disabled
3) These vairables might be set, but invalid

So, it would be nice to have a function called "salsa_api_is_active" or something, which would attempt to connect to Salsa using the current settings and makes sure Salsa is returning a valid response, and then simply returns TRUE or FALSE without throwing an error.

(This wouldn't work in all cases, since it's possible for some scripts to be working while others are not...but it would be a first step at least.)

Comments

codewatson’s picture

codewatson commented
Assigned: Unassigned » codewatson

Hmm good point, well right now i have it set so that if an error in the query occurs it sets a drupal message (containing the return error from salsa) and the salsa_api_query() function returns a value of 1 (not sure why i set it to 1.. but regardless its not an array like a successful query). However this doesn't take into account if salsa cant be reached or if the wrong login information is used.

Part of the problem is that if the wrong login information is used it seems to still pull valid data from some test database or something, so for many of the scripts valid xml is returned. Perhaps in addition to requiring the node, email and password information we should ask for the organization key as well, as everything returned from salsa contains the organization key at the beginning of the xml. Then i would just preform a check when they are filling out that field, run a simple test query and see if the org keys match. That should at least solve the wrong login issue.

I'll think some more about connection issues and see what i come up with.

codewatson’s picture

codewatson commented

Some thoughts:

Did some experiments on authentication to see what happens with wrong login information. One way i found to deal with this is based on what is returned from salsa during authentication (before running a query). If authentication fails, i close the curl connection and return a null value to the query function. If it gets a null value, it returns null as well.

This means that all you have to test for is if the value returned from salsa_api_query($query) is null or has a value. This is true for the node, username and password. I also added a check to the salsa config page that runs a test query based on the information entered and splashes a message telling the person if they have entered in the correct information or not. Thoughts?

As for if salsa is down, i'm not sure how to test for this as we cant really ask salsa to down themselves.... I do know that the curl settings are set to time out at 10 seconds, which means that if it can't be contacted it will still allow the rest of the page to load. I think the value returned is probably null, but not always, a partial value could be returned.

As for if the api is disabled, well, pretty much if your module uses the api, the api module should be set as a dependency of that module so that it cant be enabled without the api being enabled as well. Do you have a specific reason to have it disabled?

gengel’s picture

gengel commented

This sounds like most of what we need to handle. I guess I mostly just wanted one function that runs through all the necessary checks and only returned TRUE if everything worked. (Settings filled out, Authentication passes, Valid Curl/HTTP response without error code within time limit, does a generic test query return the expected results)

The broken dependency tree case is rare enough that we don't really need to handle it, you're right.

I can do a patch if you'd like?

codewatson’s picture

codewatson commented

Yea feel free. Were you wanting a test query that would run before each normal query or something that you could run before running an actual query?

gengel’s picture

gengel commented

There are some occasions where I'm running through a whole host of queries in sequence. (Syncing a bunch of tables for the first time, for example.) If these are going to fail, it makes sense to just skip the whole process rather than go through the process of each query dying and generating error conditions.

codewatson’s picture

codewatson commented

Fair point, i think its a bit of a double edged sword there though? What if you test it, it comes back fine, so you start your bulk queries and in the middle of that salsa goes away? Each query is it's own connection, so there's no guarantee that consecutive queries will run without problems (though if things are fast enough this shouldn't be a problem very often). Perhaps we need 2 tests? A general test query before any queries are run and then some logic for each actual query? (sounds like what you were looking for?)

codewatson’s picture

codewatson commented
Status: Active » Closed (won't fix)

Closing, re-open if you think we should discuss this based on current code!