consolibyte / quickbooks-php

QuickBooks Integration for PHP
Eclipse Public License 1.0
455 stars 337 forks source link

forcing database is not ideal #171

Open TimOgilvy opened 7 years ago

TimOgilvy commented 7 years ago

This repo would be much more useable if an interface was used to require a token store object with a store() and retrieve() method, allowing developers to decide where and how their token will be stored, and use their own encryption.

Providing a default method is lovely. Forcing people to use it isn't.

consolibyte commented 7 years ago

You should be able to implement this very easily actually. All of the actual DB stuff is pretty abstracted away from the actual SQL.

Did you try attempting to write your own separate Driver backend yet? It's not well documented, but should be very easy (and I can probably scrape together some documentation if required). https://github.com/consolibyte/quickbooks-php/tree/master/QuickBooks/Driver

Where are you attempting to store the data, out of curiosity?

TimOgilvy commented 7 years ago

Rad! Thanks for the prompt response.

Your repo looks much better in general than the SDK quickbooks are supplying themselves.

I'm currently just jamming stuff into the session, but I'm building it into a laravel application and will use an eloquent model and encryption to store as soon as I get out of the sandbox.

I guess I got distracted by it being called $db - if I can use the session for now inside my driver, so much the better. Obviously that isn't secure as a long term solution. Thanks again for the prompt response!

TimOgilvy commented 7 years ago

I would consider decisions about storage to be something which happens on an application level - in that sense this library is very opinionated about token storage and how it should happen. I would much prefer to be able to abstract it even further. It's not that the approach taken here is terrible at all - it's just that it means integration with a paradigm like laravel/eloquent is a little more fiddly.

consolibyte commented 7 years ago

I don't think sessions aren't really going to be any sort of viable solution here -- the OAuth tokens have to be long-lived, and have to be able to be accessed/updated by an out-of-band, not-tied-to-a-user-doing-something process to be persisted beyond 6 months (e.g. by a cron process). That sort of negates the use of sessions I would think, right?

If you're using a database anyway, would it then be easier to just use the built-in database stuff?

consolibyte commented 7 years ago

I would agree with that assessment, it'd be nice to have pluggable backends like this.

Are you developing for QuickBooks ONLINE, or QuickBooks FOR WINDOWS?

They have significantly different requirements as far as what data needs to be stored/persisted.

TimOgilvy commented 7 years ago

Sure - your right that it might be easier to implement - but it doesn't allow me to easily manage multiple scaleable instances of my server. The Laravel Migrations engine makes sure all the correct tables are spun up in local, staging, production etc. Also I will be integrating with multiple API's and I want to use the same basic structure to manage all of them.

Developing for QBO. I'm a REST junkie who prefers JSON data in general so all this XML is not my happy place, but once it's PHP objects its a moot point, and clearly intuit have only really started to accomodate JSON. Having an SDK is great, and I'm looking forward to testing yours for this task as the level of error reporting and debug logic on the intuit SDK is not amazing.

If I get a chance I'll try to have a look in more detail about how your drivers work, and possibly submit a pull request or add a laravel package which abstracts this. Only fair for me to do some work if I want the feature :)

TimOgilvy commented 7 years ago

Also stoked that you are around for the conversation - it looks like the PHP developers at intuit are not really that actively engaged in maintaining their repo - which is probably to be expected of a corporate github account I guess.

consolibyte commented 7 years ago

Gotcha, that's a great use-case. I'll see if I can whip up an example too.