Do not generate special purpose IPv4s #681
Conversation
|
How about the Loopback, shouldn't we exclude that too?
|
|
Could you please add a unit test? |
Do not genate 255.255.255.255 (Broadcast) and IPs between 0.0.0.0 and 0.255.255.255 (0.0.0.0/8, current network).
|
@fzaninotto are those tests OK for you? Writing test cases for random data feels strange 😼 What do you think about @steampilot 's proposal about also exluding 127.0.0.0/8? |
| @@ -108,6 +108,16 @@ public function testIpv4() | |||
| $this->assertNotFalse(filter_var($this->faker->ipv4(), FILTER_VALIDATE_IP, FILTER_FLAG_IPV4)); | |||
| } | |||
|
|
|||
| public function testIpv4NotLocalNetwork() | |||
| { | |||
| $this->assertNotEquals('1.0.0.0', long2ip(ip2long($this->faker->ipv4()) & (-1 << (32 - 8)))); | |||
There was a problem hiding this comment.
I don't understand your binary voodoo here.
There was a problem hiding this comment.
It shifts away the last three bits (B, C,& D from A.B.C.D), so only the first bit (A) remains.
This way we can compare it.
Probably there is a more efficient way, but I'm no binary calculation hero 😼
There was a problem hiding this comment.
ip2long($this->faker->ipv4()) >= 0x01000000 will disallow IP addresses starting with 0x00 (i.e. starting with 0.)
There was a problem hiding this comment.
@TimWolla you mean like this?
$this->assertTrue(ip2long($this->faker->ipv4()) >= 0x01000000);
There was a problem hiding this comment.
How about using a regex instead?
There was a problem hiding this comment.
@fzaninotto I think @TimWolla 's proposal makes and is certainly the fastest of all.
There was a problem hiding this comment.
@TimWolla why less? 😕
I inversed the order, as it is expected and then actual in PHPUnit.
There was a problem hiding this comment.
It is not maintainable. Even you guys don't agree on it. Please switch to a regex assertion.
There was a problem hiding this comment.
It is not maintainable.
That's a bit exaggerated, but fair enough.
How about $this->assertNotRegExp('/\A1\./', $this->faker->ipv4());
This checks if the IPv4 starts with 1., so 10. does not fail.
|
Testing random data is OK - the CI isn't seeded so its runs with new random data every time. I think your two cases are sufficient for exclusion. |
|
@fzaninotto done. |
|
Awesome, thanks. |
Do not generate special purpose IPv4s
Do not genate 255.255.255.255 (Broadcast) and IPs between 0.0.0.0 and 0.255.255.255 (0.0.0.0/8, current network).
https://en.wikipedia.org/wiki/IPv4#Special-use_addresses
https://en.wikipedia.org/wiki/List_of_assigned_/8_IPv4_address_blocks
And 255.255.255.255 (Broadcast) must not be used, also.