Warning seems good. Isn't it better to fix it in RequestFactory?

It did trigger warning on travis, but on localhost, it just hang. For minutes it did nothing so I had to kill it.

Yes, fixing RequestFactory is probably the next step, but this is a bigger problem imho.

I do not understand you…

I have probably something wrongly configured on localhost so instead of throwing warning, the function call just hangs and waits (more than few minutes) for response that is never returned. That's why I think fixing it just in the RequestFactory is wrong. It should be fixed on both places and I will push another commit with fix also for RequestFactory.

👎

You should not expect Helpers::ipMatch to work with invalid arguments. It's responsibility of the caller to pass valid arguments. Also exception would be more appropriate here instead of just returning FALSE.

@JanTvrdik exception is fine with me. Hanging on network call for minutes is definitely not fine with me.

It's responsibility of the caller to pass valid arguments.

Then why do we have frameworks that tell us what we did wrong? Shouldn't I just know? :P

Honestly, I still don't understand this issue. Is it a PHP bug? Is it a RequestFactory bug? Or did you just shoot yourself in the foot and came here to cry?

Or did you just shoot yourself in the foot and came here to cry?

@JanTvrdik that's really great attitude that will definitely make people wanna contribute to Nette.

Even if I did. Isn't it more of a reason to solve this case so nobody else has to encounter it?

Yet you did not answer my question. I still don't understand this issue. I could have offered you an advice how to best solve this have I understand the problem.

This PR means: place @ in code to pass my tests.

Rejected.

No, it does not. I'm trying to reproduce it in my code right now and fix it in RequestFactory.

• hanging on network should be reported on bugs.php.net.
• checking for empty values is IMHO unnecessary, 'xxx' is invalid argument too.

hanging on network should be reported on bugs.php.net.

And wait >2 years till somebody fixes it? I will definitely report it to PHP (if I manage to reproduce it, which I'm having a trouble right now).
But I do not understand your logic. Nette is full of workarounds for PHP bugs. How would this be different?

checking for empty values is IMHO unneccessary, 'xxx' in invalid argument too.

Does checking for empty (and not calling ipMatch at all) in RequestFactory makes sense to you? I don't really use the function directly, so I don't really care on what level it will be fixed, as long as it will be fixed.
Would it be better to throw an exception from RequestFactory, or simply ignore it?

Nette is full of workarounds for problems that can be reproduced ;-)

Nobody is pushing you to merge this right away. As I've said earlier, I'm trying to reproduce it.

ipMatch returns FALSE and E_WARNING for invalid arguments, it is not the best, but good. Your first PR suppressed E_WARNING for some of invalid arguments (and for others not), your second PR escaled E_WARNING to exception for some of invalid arguments (and for others not). This is mess.

Nobody is pushing you to merge this right away.

Pull Request.

Hele já už to anglicky nedávám... pojďme se prosím bavit konstruktivně a vynechat trolování a slovíčkaření...

Od té doby co mi to na tom volání mrzlo jsem musel restartovat počítač a teď to nejsem schopnej zreprodukovat. To nic ale nemění na tom, že ta funkce furt hází ten warning což je pro mě problém.

Současná situace: když pustím testy v cli (ať už přes php-cgi nebo php) tak $_SERVER['REMOTE_ADDR'] je prádzný a tedy se do Helpers::ipMatch() dostane NULL.

Návrhy řešení:

• 1. budou se kontrolovat argumenty Helpers::ipMatch(), buďto:
  • 1a) dá se před volání inet_pton() @ a když vrátí FALSE
    • 1aa) tak to vyhodí výjimku
    • 1aa) celá funkce vrátí FALSE
  • 1b) volání inet_pton() se obalí do vlastního error handleru jako je například \Nette\Utils\Callback::invokeSafe a když funkce vyhodí warning
    • 1ba) tak to vyhodí výjimku
    • 1ba) celá funkce vrátí FALSE
• 2. fixne se to na úrovni RequestFactory - když je $_SERVER['REMOTE_ADDR'] empty, tak se nebude kontrolovat proti proxynám
• 3. fixovat se to nebude, protože je to "validní chování" a když je $_SERVER['REMOTE_ADDR'] empty a zároveň jsou nastaveny proxyny, tak chceme aby aplikace házela warningy, takže si v bootstrapu testů nastavím natvrdo $_SERVER['REMOTE_ADDR'] = '127.0.0.1' a napíše se to i do dokumentace, protože určitě nejsem první ani poslední člověk co potřebuje vytvářet DI Container v testech.

E_WARNING is fine for me, but if you need exception, put @ before inet_pton. You can throw exception instead of return FALSE in condition.

ping @fprochazka