Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for S1lentium/IPTools#15 #18

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

petski
Copy link
Contributor

@petski petski commented Mar 17, 2017

IPv6 blocksize can exceed the maximum number for integer (2^32 or 2^64).

This commit fixes $network->count() [1] (by returning the number of hosts as a string if it exceeds the max integer size). It can't fix count($network) [2], since PHP's count() must return an integer.

[1] Or $range->count()
[2] Or count($range)

@coveralls
Copy link

coveralls commented Mar 17, 2017

Coverage Status

Coverage remained the same at 96.491% when pulling 399b553 on petski:master into 5ce333e on S1lentium:master.

@S1lentium
Copy link
Owner

S1lentium commented Mar 23, 2017

Yes, this fix. But it breaks concept of the Countable interface. Or you can delete this interface, and check count methods on other classes.

@petski
Copy link
Contributor Author

petski commented Mar 23, 2017

It's not that my patch breaks anything. My patch actually makes the issue described in this bug-report work for $network->count(). The issue indeed remains if the Countable interface count($network) is used, due to limitation of PHPs count() method...

I agree that dropping the Countable interface is probably the best idea. Unfortunately, that will break backwards compatibility. Users should be informed about this in the upcoming release.

Let me know if you indeed want this approach. I will update my patch accordingly (or you may do so, if you want)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants