mstenta / farm_sync

Drupal 8 module with features for synchronizing records from farmOS into the local database.
1 stars 0 forks source link

Automatically authenticate in farmOS class constructor? #4

Open mstenta opened 5 years ago

mstenta commented 5 years ago

This is up for debate: should we automatically authenticate inside the farmOS class constructor? Or is there reason to leave the authentication action as a separate step? Are there cases where you would want a farmOS class but you wouldn't want to call authenticate()?

mstenta commented 5 years ago

Idea: it could be an additional boolean argument in the constructor:

public function __construct($hostname, $username, $password, $authenticate = TRUE) {

jgaehring commented 5 years ago

Idea: it could be an additional boolean argument in the constructor

+1

jgaehring commented 5 years ago

So far with the JS version, the authentication and actual requests are happening in very separate parts of the code. So that might be an argument against doing it automatically.

Also, I've gotta play with it more, but I might require the consumer to keep track of the token after authentication, and then pass it back in for record requests. I suppose automating the authentication could be one way of avoiding that extra step for the consumer; they might never have to worry about the token if authenticaion is part of every record request, but that seems like overkill.

mstenta commented 5 years ago

Yea that's something I've thought a little bit about with the PHP class too. After authentication it stores the token in an internal class property - so it gets reused as long as the class instance is alive. But, an instance only lives as long as a PHP page request, so multiple requests means multiple authentications. I can see that in the Drupal logs for the site I'm testing against... multiple logs saying "Tester has authenticated". This isn't ideal, but also not destructive in any way, as far as I know.

So yea, in the long run it's sort of the responsibility of the user to keep track of the token and use it responsibly within their own app. But right now that's not really possible in the PHP class because you can't create a new instance with an existing token. This could be added as a new argument or method, however. Also, the cookie is required too... not just the token.

Something like:

$farm = new farmOS();

// If we already have a cookie and token stored, plug them in.
if (!empty($cookie) && !empty($token)) {
  $farm->setCookie($cookie);
  $farm->setToken($token);
}

// Otherwise, authenticate.
else {
  $farm->authenticate();
}