search

ActiveCampaign / activecampaign-api-php

MIT License
115 stars 76 forks source link

Option to disable sending header http #30

Closed dieend closed 8 years ago

dieend commented 8 years ago

Add options to disable header options (defaults to enabled for BC). I have my own exception handler that will set the http code at other place in requests. This header line broke my exception handler. And it truly broke those who uses rpc (such as thrift) that must not have HTTP 1.1 in the header.

It should also fix #28 (which also happened to me and fixed by commenting out line 205 in Connector class). Actually I don't have any idea why you have this header line.

ValerioM commented 8 years ago

+1 to this, it's just plain annoying and useless.

chrisrichard commented 8 years ago

This issue makes it painful to use from PHP-CLI or after the response has been flushed. We have to manually comment out the line which is not durable and very error-prone.

Please merge this PR ASAP or better yet, remove this line altogether as it should not be there in the first place.

pevans commented 8 years ago

Apologies for getting back to this issue so late... we actually removed the header() call here, so there is no need to merge the pull request. We goofed on updating here.

@QWp6t I agree that the documentation here is not where we'd like it to be. I do assure you that this repository is maintained.

Overall we're looking to launch a new API version (and wrapper class) that is fully RESTful and JSON-API compatible. We're still in the process of bringing up full feature-compatibility in the latest version; some endpoints are still missing.

chrisrichard commented 8 years ago

@ac-pevans I still see the header call here:

https://github.com/ActiveCampaign/activecampaign-api-php/blob/master/includes/Connector.class.php#L213

This was a PR for #28 but removing the header call is definitely preferable. This wrapper should not be concerned with setting the response code of the SAPI request which may or may not exist.

pevans commented 8 years ago

That's bizarre... ok, I could have sworn that was out. But you're right--there it is.

This commit should remove it for you.

warkior commented 8 years ago

Any chance you folks could release a new tag for this? (release) When installing with composer it pulls release 1.1.0 which still seems to have this bug. (3 mo later)

pevans commented 8 years ago

You're right about that--I have tagged the change as v1.2.0, which you should see now.