Zend Framework

Attached tiny reproduction script using Zend_Validate

Tests are partitially not reproducable:

Example:
10.0.0.1 is a valid IP4 address.
According to your testbed it is invalid.

Do you have further informations ?

So basically you are saying that PHP's native functions do not work and ZF should fix a PHP bug ?

See this bug report within PHP:
http://bugs.php.net/bug.php?id=50117

I'm not sure if ZF should try to fix PHP bugs.
Note that changing this implementation from PHP to ZF will have a massive performance impact.

Note additionally that this bug report was reported "9.Nov."... at the same day as this issue was reported to ZF.

I removed the ipv4 address, that was obviously my bad (hey, it was late!).

Furthermore I do think we should fix this, no matter if it's PHP's behaviour that is wrong here, or ZF's. ZF claims to validate ipv6 addresses, and currently it does not do that entirely right. If you look for example at Zend_Validate_Int, we could just have gone for return is_int(), but because we wanted to do more than php itself can, we implemented the extra functionality ourselves. Same goes for NotEmpty, and probably more.

As to what the performance impact is concerned; I don't think there would be much. We would only have to think of a regex which takes care of the abovementioned ipv6 addresses and check them only if the other 2 IP-checks fail.

Fwiw; I did not have any contact with the author of the url's that are in the original report, so it's kind of coincidence that we report the same issue within hours from each other.

I attached a patch that does most of the abovementioned ips correctly. It also contains a (commented out) testcase with ip addresses php currently does do wrong, which we could later use when php bug #50117 is fixed.

EDIT: Patch redrawn. I have got a way better patch that should fix all problems. However, because not all code is mine, I am waiting for approval first.

Fixed with r18906.

Special thanks to Tino Zijdel ( http://crisp.tweakblogs.net/ ) whose code we were allowed to borrow.

A question of mine does not mean to take over the issue yourself and implement this without any further notice.

Next time, before taking over an issue, please ask the maintainer even if he asked you a question.

Please forward me the mail from Tino related to "allowed to borrow" according to our NDA rules.

Additionally you stated in your comment to use old and new implementation as fallback for performance but it was not implemented this way.
Have you made any performance checks related to your reply ? How are your results ?

Need to check code and it's rights

Thomas – Dolf has acquired rights to contribute the code (and I have seen the emails). As such, the onus of responsibility lies on him as he committed the changes. Focus on the other questions instead.

Thomas, my original patch relied on filter_var() which may not be installed on each system, therefore I used the original implementation with inet_ntop() as a fallback. However, because my new fix (which I committed to trunk) does not rely on any extensions that may, or may not be installed, there's no need for any such fallback.

Then there's the issue of performance; I did not benchmark my fix against the old implementation. The reason why I neglected to do so is because the old implementation is flawed (hence this issue) and is never going to work for each ipv6 address. The main problem basically is that inet_ntop() only returns IP's in their standard notation (which is compressed). However, when validating a non-compressed IP, it will be be outputted as compressed (if possible), and therefore always differ from the original input (invalidating it where it actually is valid). Though noncompressed IP's are no standard notation, they are entirely valid acc. to the relevant specs. Besides that, with the new implementation only one (tiny) regex is used max, so there can never be a noticeable slowdown.

The reason I committed it to trunk was because I am pretty certain of my fix being right since all unittests pass, my test actually adds 15 assertions.

<|thomas_1> Problem with I18n is that most people only think of THEIR environment or server settings and not is I18n as complete API with effect to about 20 components
<|thomas_1> He attached an issue from me to himself, ... without a solution for 4 days... no good behaviour in my eyes..

This obviously is not an environment/server setting, and I did introduce a solution within minutes after assigning it to myself. Besides that I made sure all unittests passed (to make sure trunk was still 'stable'), and I did not commit it to any release branch so you would have the option of reviewing/changing stuff.

Fwiw; I never signed an NDA for ZF related stuff, only a CLA.

p.s. I could forward you that conversation, but I am not convinced of any such need, and like to keep emails as confidential as possible (which is why I forwarded them to Matthew only).

I just verified the performance of the new implementation.

I created a testbed which tested 6500 IP addresses.
These tests were run 5 times to have a good overview.

The old implementation needed a average time of 3.2 seconds per run.

The same tests with the new implementation needed a average time of 2.1 seconds per run.

This means that the new implementation is not slower but even faster as the native PHP implemenation by about 30-35%.

Note that I did this tests within a Win* development environment. On *nix this should even perform better.

Therefor closing this issue as fixed. Great work

Tested by using the adresses from the patch, and additional ones, multiple times.