leth/PHP-IPAddress

Required unstable PEAR library

Closed this issue · 5 comments

denis-sokolov commented

Your documentation claims that PEAR bigint library is optional, but your composer.json requires it. Even more, the version you require is not considered stable, so to install leth/ip-address one needs to explicitly require an unstable version of bigint.

I wanted to submit a pull request making bigint a suggest in composer.json, instead of a require and simply throw exceptions if user tries to access features that require it.

But that would make your library not backwards-compatible with existing clients who have minimum-stability set to dev in their composer.json. Your new version would imply that bigint is no longer required, and if your users do use the functionality, it will suddenly begin throwing exceptions. Of course, clients can easily fix it by adding an explicit require for bigint.

Depending on your attitude towards backwards compatibility this may or may not be an option, what do you think?

leth commented

Hmm, I'm not sure which option to choose here:

  • Fix the documentation
    Pros:

    • Simple fix
    • Everything works out of the box

    Cons:

  • Change to suggest
    Pros:

    • Still an optional requirement

    Cons:

    • Break backward compatibility (release with dependency has only been out for 9 days)
    • Not everything works out of the box
    • Not tested what happens if BigInteger fails to autoload

I'm leaning towards just fixing the documentation, particularly because I've not tested how it behaves without the package.
Is the added dependency a problem? (I don't really do proper PHP development these days, so I'm not really exposed to this)

If having it as an optional dependency is critical for you I don't mind breaking the backwards compatibility this once, providing:

  • There is a travis test matrix entry for running without BigInteger (might need to skip some tests)
  • If some voodoo is needed to satisfy the autoloading problem, then we log some kind of warning message.
leth commented

I've opened a bug about the BigInteger releases: https://pear.php.net/bugs/bug.php?id=20242

denis-sokolov commented

It's not critical for me, but it's highly inconvenient to require your library currently.
One can't just require leth/ip-address, the weird BigInteger version must be specificied manually.

Also, consider that 9 days is a very short time, you can drop the dependency right now.
If you don't, you're stuck with it forever. So you have an awesome opportunity right now.

Travis test matrix entry is possible. Just conditionally install based on an env variable.
As for the autoload, I don't see why it should be a problem. In your code you use BigInt class name in two ways - instanceofchecks, those just work, and new, that can be trivially replaced with if (class_exists) new else throw.

tabacco commented

Hey, just a note that pear has created actual tags for their releases, so you could potentially require "pear/math_biginteger": "1.0.2" instead.

leth commented

Hopefully everything is sorted now!