socrata / soda-php

A simple library to make it easier to access SODA services from PHP
http://dev.socrata.com
Other
64 stars 28 forks source link

Don't die() on me #23

Closed sanderdekroon closed 8 years ago

sanderdekroon commented 8 years ago

Please remove the echo and die() statements from your library. It makes no sense whatsoever to let PHP die() is there's an unexpected result.

Instead, return an array with an error/error code or throw an catchable exception.

A simple solution would be:

if($code != "200") {
      return array('status' => false, 'statusCode' => $code);
}

That way we can simply check the response from the function with isset($response['status']) and retrieve the status code if we want to.

Alternatively you could implement a custom Exception and throw that instead.

allejo commented 8 years ago

Are you using the latest master? This was fixed in 208d16b013ea58b59655df130be4e0cbc020ab62

sanderdekroon commented 8 years ago

Huh, that's odd. I downloaded the library last week via composer, but apparently not the latest master.

chrismetcalf commented 8 years ago

@sanderdekroon, @allejo Did I not update something properly in Composer? I replaced the die()s with exceptions, but it wouldn't surprise me if there wasn't some other packaging detail I missed.

allejo commented 8 years ago

Composer uses tags for stable releases by default so unless someone is using dev-master explicitly in their composer.json, they'll be getting the last tag which doesn't include your changes.

On Oct 8, 2016, 11:39 AM -0700, Chris Metcalf notifications@github.com, wrote:

@sanderdekroon (https://github.com/sanderdekroon), @allejo (https://github.com/allejo) Did I not update something properly in Composer? I replaced the die()s with exceptions, but it wouldn't surprise me if there wasn't some other packaging detail I missed.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub (https://github.com/socrata/soda-php/issues/23#issuecomment-252440952), or mute the thread (https://github.com/notifications/unsubscribe-auth/ABME9fjBu2-3U7AcJTYpX791-5Uk_S0uks5qx-NegaJpZM4KQx1g).

chrismetcalf commented 8 years ago

@allejo GitHub release tags? So if I cut a new tag in GitHub that'll work?

allejo commented 8 years ago

Yea, a newer tag will allow users to pull that as the current "stable" release instead of having users always pull the latest master (which could possibly be under development). The latest "stable" is currently 1.0.5.

chrismetcalf commented 8 years ago

@allejo Like this? https://github.com/socrata/soda-php/releases/tag/v1.1.0

@sanderdekroon Let me know if the 1.1.0 release works for you now.

allejo commented 8 years ago

@chrismetcalf that should do it 👍

Just target 1.1.x in your composer file and run composer update.

chrismetcalf commented 8 years ago

Cool, I like how that works! Thanks for your help as usual, @allejo! 🍻