[HttpFoundation] Move IP check methods to a HttpUtils class for reuse #6005
Conversation
| { | ||
| if (false !== strpos($requestIp, ':')) { | ||
| return self::checkIp6($requestIp, $ip); | ||
| } else { |
There was a problem hiding this comment.
no need for else here as the if returns
|
Thanks @stof ! (didn't get my copy paste error as PHP allow calling non static method w/o a warning). |
| * | ||
| * @author Fabien Potencier <fabien@symfony.com> | ||
| */ | ||
| class HttpUtils |
There was a problem hiding this comment.
Make the class final, for restricting it to be inherited.
class MyHttpUtils extends HttpUtils
8000
{
public function __construct() {}
}
// In Java you cant inherit from class with private default constructor,
// but in PHP you can
$utils = new MyHttpUtils();
There was a problem hiding this comment.
@vicb, I saw it, but I don't get any point!, you change the access modifier to private to make the class not instantiated, but some one could inherit from the class and make an instance object of inherited class without calling the parent constructor, and it is bug.
There was a problem hiding this comment.
The change is for consistency with other ...Utils classes. Having a private constructor is a safeguard to prevent from instatiating a class with static methods only. Users can still extend the class as it is not final (in such a case the safeguard will not be useful any more, is that really important ?)
There was a problem hiding this comment.
Thanks for your point. No, when the programming language is PHP, no, it is not really important (In OOP when some class has private constructor is not extendable and instatiable)
Thanks for answering my comment, I get the note.
|
Having an |
|
@fabpot could this be merged if |
| * @param string $requestIp | ||
| * @param string $ip | ||
| * | ||
| * @return boolean True valid, false if not. |
There was a problem hiding this comment.
True if valid, false if not ?
|
Renaming the class to |
|
ready ! |
|
Can you add an entry in the CHANGELOG? |
|
done, thanks for the reminder ! |
No description provided.