Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Api host and Api Version by parameter, to support multiple (host … #385

Conversation

mahsanamin
Copy link

This addition will allow library to support any kind of Host, the main reason for now is to support the Ads Api URLs. I tried to keep it ad dynamic because it was less change and will allow the developers to think on their own regarding (Sandbox url and version). And this change will also leave system backward compatible.

The other important point is the addition of apiVersion inside http( method, I rearranged the parameter sequence because it is private method! and I think apiVersion should be in-front of host.

It was possible to follow the existing convention and put the URL as constant inside library but I think at this point we just need to add another flexibility to provide any host parameter and its handy to give Sanbox Url of Ads this way.

Note that I also added the test and tested those which works fine!

@abraham
Copy link
Owner

abraham commented Jul 22, 2015

This isn't going to scale very well because it'll potentially need putWithHost, postWithHost, deleteWithHost, uploadWithHost, etc. I think a better approach would be to use child objects like this:

$to = new TwitterOAuth(...);
$to->ads->get(...);
$to->adsSandbox->get(...);

@mahsanamin
Copy link
Author

Thank you Abraham for viewing it. I understand your concern! And I also saw your above code in your Github issues, I agree that it would be a very clean way to use your library but keeping backward compatibility may also be difficult.

Also it look like a little big change! The reason I did above way is to keep change as less as possible and still allow the support of Ads, at max we need to add two more small functions putWithHost, deleteWithHost which I didn't added because those were not available in Tag 0.5.3.

One note: you don't need upload method in case of Ads because the upload function is similar for twitter Ads!

@mahsanamin
Copy link
Author

Abraham, I edited my comments above because those were little misleading and I had little confusion which I understand now! Let me know if you need my help to implement your way.

However I still feel that adding the methods like I did is the easiest way to add support of Ads and it won't be breaking!

Thank you!

@abraham abraham added this to the v2.0 milestone Feb 23, 2017
@abraham
Copy link
Owner

abraham commented Feb 23, 2017

Ads support will be planned for v2.

@abraham abraham removed this from the v2.0 milestone Jul 3, 2018
@abraham
Copy link
Owner

abraham commented Jul 4, 2018

This is going to be handled in a more generic way in #684

@abraham abraham closed this Jul 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants