iranianpep / darksky Goto Github PK

@iranianpep thanks for your work here.

I was wondering why you decided to require LAT and LONG in the constructor, since in my opinion they are relevant to the actual data request.
I personally need to perform several requests for different locations so I'd see more appropriate something like:

$darksky = new Darksky('API_KEY');
$forecast = $darksky->forecast('LAT', 'LONG');
$historic = $darksky->timeMachine('UNIX_TIME', 'LAT', 'LONG');

which would allow to avoid to instantiate a new Darksky instance for each call.

What do you think?

P.S. BTW the docs have a typo in $result = $darksky->forcast();

Even though it seems to me your library does not have other dependencies, your composer.lock for some reason still requires a bunch of dependencies among which guzzle/guzzle, which is abandoned:

It looks like this package uses the file_get_contents to request the remote url, but I think we need to more options when doing HTTP request.

I recommend using the curl_* functions or Guzzle can solve this issue.

Once we use the HTTP library, we can get the complete response information. For example, the response HTTP status code, response header, response body and so on.

We can also change getFileContent method name to another method name.

How about naming getResponse method?

The RequestMethod::submit should be enhanced because the return value is not good enough to users.

If the users use the GuzzleGet class to do HTTP request, it will return the GuzzleHttp\Psr7\Response object. I think it's the flexible way to let users get response headers and HTTP status code by themselves.

If the users use the Get class to do HTTP request, it will only return the response content.

They cannot receive any HTTP response headers and status code via submit method.

But it's possible to use file_get_contents to get HTTP headers and status code.

Please consider this official PHP reference and we can know using $http_response_header to get response HTTP headers after calling file_get_contents.

Maybe we can return array when using Get::submit method?

@iranianpep, do you have any idea about this? Thanks.

Hey @iranianpep,
I noticed that the file_get_contents function you use to fetch data throws a warning if the source does not exist, for example if the passed url is not reachable for some reason (like in my case).
Unless you handle warnings with a custom set_error_handler you basically can't catch this type of error and easily handle the case in which the connection isn't available.

I'd suggest to silence errors when using the function in your code (i.e. @file_get_contents()) and check the returned value, if it is false throw a DarkskyException for connection or fetch error.

Hey @iranianpep,
I'm having an issue with Darksky API when running my app on Heroku, it always throws this:

Severity: Warning --> file_get_contents(https://api.darksky.net/forecast/2b45ce1648***********/47.225451000,-119.956826000,2018-04-10T00:00:00-08:00?lang=en&units=us&exclude=currently%2Cminutely%2Calerts%2Cflags): failed to open stream: HTTP request failed! HTTP/1.1 502 Bad Gateway /app/vendor/darksky/darksky/src/Darksky/Darksky.php 83 

Sometimes the HTTP/1.1 502 Bad Gateway part is not there, so I guess another error type occurs.

Do you have any clue of what might be happening and how to fix it or find a workaround? It basically does not work in there, locally and on the browser the urls (and the library) work fine instead.

As title, using the file_get_contents to do HTTP request will have some vulnerability problems.

It explains the security issue in the humbug/file_get_contents introduction.

We can also consider how to use file_get_contents to do HTTP request securely in get method.

This issue should be concerned.