maxmind / GeoIP2-php

PHP API for GeoIP2 webservice client and database reader
https://maxmind.github.io/GeoIP2-php/
Apache License 2.0
2.33k stars 276 forks source link

GeoIP2-php requires a deprecated dependency #38

Closed MiguelCP closed 9 years ago

MiguelCP commented 9 years ago

GeoIP2 package uses a deprecated library. Installation via composer shows the following message:

guzzle/guzzle suggests installing guzzlehttp/guzzle (Guzzle 5 has moved to a new package name. The package you have installed, Guzzle 3, is deprecated.)

oschwald commented 9 years ago

Thanks. We cannot move to Guzzle 5 as it no longer supports PHP 5.3. We may move away from Guzzle in the future.

trsteel88 commented 9 years ago

Would love to see this library updated to use the latest Guzzle. PHP5.3 is quite old and I don't think this library should be held back by that requirement.

I think it would be great if the models were updated to use getters/setters for all data too (e.g $record->getCountry()->getIsoCode() instead of $record->country->isoCode)

oschwald commented 9 years ago

5.3 is by far the most popular PHP version currently.

As for getters, there isn't really an encapsulation advantage to using them.

trsteel88 commented 9 years ago

Stats by Wordpress beg to differ: https://wordpress.org/about/stats/

Newer versions are quickly becoming more and more popular with requirements set by frameworks such as Symfony2.

The main advantage of the getters/setters is to create a clear api. It also has the added advantage of allowing ide's such as phpstorm to autocomplete code for you.

oschwald commented 9 years ago

w3techs has 5.3 at 42%. Even if we go by the WordPress numbers, 35% is not a smaller number.

As far as the getters go, I find $record->country->isoCode much easier to read than $record->getCountry()->getIsoCode(). Regardless, replacing the properties with getters would be a breaking change that would require a major version bump and is unlikely to happen.

trsteel88 commented 9 years ago

Not sure where w3techs is getting their info from. I would be much more inclined to trust Wordpress.

It would be a major version bump. However, this would also also the php 5.3/guzzle dependancy. Leave the current version for PHP 5.3 and start a new major version depending on PHP >= 5.5

It may be easier to read, but it doesn't create a clear and concise api. I think most developers would open the models and look for methods to call. At the moment, every time I come back to use the library I have to re-read the documentation because I can't remember exact properties to call.

oschwald commented 9 years ago

As I mentioned to the original poster, we will likely just replace Guzzle with using cURL directly in the future.

The model classes have extensive documentation for each property. Most IDEs that I know of also pick up on the use of @property in the phpDocs and will auto complete as is. I think we will just have to agree to disagree about which one is clearer and conciser.

trsteel88 commented 9 years ago

I think locking the system into curl is a bad idea. You want to keep it as separate as possible so users who don't have access to curl can still use the system.

oschwald commented 9 years ago

Guzzle 3 already requires cURL, and I am not aware of any complaints we have received about this dependency.

oschwald commented 9 years ago

We have release v2.2.0-alpha1, which no longer requires Guzzle.

max-voloshin commented 9 years ago

Looks like v2.2.0-alpha1 version has broken dependencies:

$ composer require geoip2/geoip2:v2.2.0-alpha1
./composer.json has been updated
Loading composer repositories with package information
Your requirements could not be resolved to an installable set of packages.

  Problem 1
    - Installation request for geoip2/geoip2 v2.2.0-alpha1 -> satisfiable by geoip2/geoip2[v2.2.0-alpha1].
    - geoip2/geoip2 v2.2.0-alpha1 requires maxmind-ws/web-service-common dev-master -> no matching package found.

Potential causes:
 - A typo in the package name
 - The package is not available in a stable-enough version according to your minimum-stability setting
   see <https://groups.google.com/d/topic/composer-dev/_g3ASeIFlrc/discussion> for more details.

Read <http://getcomposer.org/doc/articles/troubleshooting.md> for further common problems.

Installation failed, reverting ./composer.json to its original content.
oschwald commented 9 years ago

The cause is:

 - The package is not available in a stable-enough version according to your minimum-stability setting
   see <https://groups.google.com/d/topic/composer-dev/_g3ASeIFlrc/discussion> for more details.

You need to add this in your composer.json:

"minimum-stability": "dev" 
max-voloshin commented 9 years ago

Oh.. my bad:( Thanks!

oschwald commented 9 years ago

I am closing this issue as the branch replacing Guzzle has been merged. We did a beta release of it and will likely do a full release by the end of the month.