Weble / ZohoClient

11 stars 7 forks source link

[WIP] v4 #7

Closed Skullbock closed 3 years ago

Skullbock commented 3 years ago

@tm1000 this is my proposal for a v4.

Docs need updating, same as examples

Thoughts?

Skullbock commented 3 years ago

Also, basic test now works on PR + master ;)

tm1000 commented 3 years ago

Overall it looks good with a few comments I have. Besides that i see you weren't able to get rid of your client completely which I'd rather have it this way (I know it means you still have to maintain the client as well 😞

Skullbock commented 3 years ago

If by my client you mean the OAuthClient class, i kept it because as you said having the cache is super useful in the other libraries, and also this way we can avoid backward compatibility issues since the public api is the same basically

tm1000 commented 3 years ago

If by my client you mean the OAuthClient class, i kept it because as you said having the cache is super useful in the other libraries, and also this way we can avoid backward compatibility issues since the public api is the same basically

Yes that's what I mean. I like having the cache access personally.

Skullbock commented 3 years ago

Agree.

Gonna push a v5 of ZohoCrmApi package with some improvements using this class, so you can check that too if you want :) If all ok, i'll release the v4 of this soon

tm1000 commented 3 years ago

@Skullbock theres two things I'd like you to apply to ZohoCrmApi one is a bug and one is a feature. I'm going to go add PRs there now

tm1000 commented 3 years ago

@Skullbock for some reason this work never got applied to v4 so I am unable to test anything we've done. I'm still trying to figure out why

tm1000 commented 3 years ago

Proof: https://github.com/Weble/ZohoClient/blob/master/src/OAuthClient.php#L452

It's still setting a TEMP token in master.

tm1000 commented 3 years ago

Ok so the issue is that develop is different from v4 which is different from master. Essentially master at v4 is broken because calling setRefreshToken immediately calls to set TEMP which I removed in #9 which was merged into this but this was never merged into v4 which v4 is merged into master. So develop should merged into v4 and v4 should merge into master then master should merge back into develop then delete the v4 branch