Refactor idea

[Kurounin]

Hey,

Thanks for the work on this library so far. I was thinking of refactoring the loading of the token logic, but wanted to run it by you first. What do you think of:

• making LoadAccessToken private and calling it from the constructor
• moving the refresh token check from HasToken to LoadAccessToken
• setting the AccessToken in the LoadAccessToken and the SaveAccessToken
• in HasToken just check if AccessToken is not null
• making the SaveAccessToken private and call it also from GetAccessToken
• making $authCode optional for GetAccessToken in which case it would just return the current AccessToken

[TecsiAron]

@Kurounin First off thank you for you contributions and courtosey!
Here are my thoughts on the changes you'd like to make.

• Move Refresh logic to LoadAccessToken - the original idea was to have the token refresh very frequently, but its not really needed as much as it is currently.
• Set access token from LoadAccessToken SaveAccessToken - Just makes sense
• Given the above changes stripping any superfluous logic from HasToken makes sense

Implementing these changes would also require a change to SendANAFRequest, as it currently relies on calling HasToken to load the token when needed.

• Set SaveAccessToken to private and also call it from GetAccessToken() - yes but maybe rename GetAccessToken to ProcessOAuthCallback (obviously to be called from the login callback script). A simple GetAccessToken () method that just returns $this->AccessToken is also a-ok with me.

Calling LoadAccessToken from the constructor would cause an extra outgoing request when it might not be necessary, not all outgoing request require a token. Given that the ANAF API apears to go up and down more than a rollercoaster, sometimes partially, I'd rather not have any requests that are not necessary, especaly not on initialization.
I think LoadAccessToken, RefreshToken and SaveAccessToken should remain public (even if called automatically) as it allows for some flexibility with things like cron jobs. It also makes debugging some issues simpler.

[Kurounin]

Thanks for the quick response.

Calling the LoadAccessToken in the constructor would only load the token from the local json file except when it needs to refresh it. This could be a problem if the refresh fails, so maybe instead of refreshing on initialization, maybe we could refresh only when using it!? we could have the GetAccessToken(bool refresh) which internally we would call it with true.

RefreshToken could stay public for calling it earlier than needed, but LoadAccessToken and SaveAccessToken wouldn't serve any use.

[TecsiAron]

Just pushed a commit that integrates some of the changes you suggested, feel free to create a PR with any further changes you'd find useful.
I did some testing, and will continue to monitor the system where this code is live to see if there are any issue, if not I'll create a release sometime tomorrow.

[Kurounin]

Thanks, the changes look good. Just did a PR to load the access token when calling HasAccessToken() and changed the UploadEFactura method to parametrize the seller cif.

[TecsiAron]

Tested the changes on a live system and all seems fine, so I went ahead and published release 1.1.0, thank you for your contributions!