@mnapoli opened this Pull Request on October 23rd 2014 Member

OK following #6494 this is the PR for the extraction of the Piwik\IP class into the Network component. I have also included IP anonymization.

Feedback is welcome!

Classes

I moved most methods into a new IP class that is not static anymore, it's a value object. Usage example:

// Factory methods:
$ip = IP::fromStringIP('127.0.0.1');
// IPv6
$ip = IP::fromStringIP('::1');
// In binary format:
$ip = IP::fromBinaryIP("\x7F\x00\x00\x01");

echo $ip->toString(); // 127.0.0.1
echo $ip->toBinary();

// IPv4 & IPv6
if ($ip instanceof IPv4) {}
if ($ip instanceof IPv6) {}

// Hostname reverse lookup
echo $ip->getHostname();

if ($ip->isInRange('192.168.1.1/32')) {}
if ($ip->isInRange('192.168.*.*')) {}

// Anonymize an IP by setting X bytes to null bytes
$ip->anonymize(2);

As you can see, there are factory methods in the base IP class. Then there are 2 implementations: IPv4 and IPv6. I know at first it sounds like unnecessary to split it in 3 classes like that, but it actually makes a lot of sense and the code is better.

Take IP anonymization for example. With a single IP class:

    public function anonymize()
    {
        if ($this->isIPv4()) {
            // do this
        } else {
            // do that
        }
    }

With IP + IPv4 and IPv6 classes:

class IP
{
    public function anonymize();
}
class IPv4
{
    public function anonymize()
    {
        // do this
    }
}
class IPv6
{
    public function anonymize()
    {
        // do that
    }
}

Polymorphism FTW!

In general, everywhere isIPv4() or isIPv6() was used, it was to implement a custom behavior. That's what OOP is for. So that's (IMO) a good replacement. And there's no impact to the user, only better code and better tests.

There were also some methods that didn't belong in an IP object, so I put them in a IPUtils class:

echo IPUtils::binaryToStringIP("\x7F\x00\x00\x01");
echo IPUtils::stringToBinaryIP('127.0.0.1');

// Sanitization methods
$sanitizedIp = IPUtils::sanitizeIp($_GET['ip']);
$sanitizedIpRange = IPUtils::sanitizeIpRange($_GET['ipRange']);

// IP range
$bounds = IPUtils::getIPRangeBounds('192.168.1.*');
echo $bounds[0]; // 192.168.1.0
echo $bounds[1]; // 192.168.1.255

Naming

As a matter of naming, I have replaced the P2N() and N2P() notation with something more explicit: binary format and string format. After looking online, "presentation format" and "network address format" where not so common terms. Hope it's OK, let me know if you have better ideas. It can be changed.

Namespace

In the new component we have now:

  • Piwik\Network\IP
  • Piwik\Network\IPv4
  • Piwik\Network\IPv6
  • Piwik\Network\IPUtils

Should it be moved to the subnamespace Piwik\Network\IP\?

I'm wondering if it's necessary if someday we add other classes to that component.

Deprecated old Piwik\IP class

I have deprecated all the methods of the old IP class that I have moved to the new component. I have also replaced almost all the usages of the old IP class by the new ones throughout the codebase. So Piwik\IP class is almost never used anymore.

However some methods where not moved:

  • getIpFromHeader()
  • getNonProxyIpFromHeader()
  • getLastIpFromList($csv)

Those are either specific to the current request or just too specific at all.

TODO

  • [x] Update the changelog
  • [x] Publish on Packagist
  • [x] Update composer.json to use a tagged version and remove the URL to the git repository
  • [ ] Search and replace usages of old Piwik\IP in public and pro plugins?
  • [x] Add tests for deprecated methods
@mattab commented on October 24th 2014 Owner

Nice to see this PR! :+1:

Here is feedback:

+1 for renaming P2N / N2P

However some methods where not moved

maybe you have a suggestion where we those methods could be moved to, or do you think it is better to leave them in IP class for now?

I have also replaced almost all the usages of the old IP class by the new ones throughout the codebase.

Sounds good that you thought of that.

When you searched for occurrence in the codebase, you can search in all repos not only piwik/piwik. you can use user:piwikpro user:piwik to match all repositories. Maybe you will find new uses of these deprecated methods?

@mnapoli commented on October 24th 2014 Member

maybe you have a suggestion where we those methods could be moved to, or do you think it is better to leave them in IP class for now?

  • getIpFromHeader() & getNonProxyIpFromHeader(): Belong to a Request class/package (see f.e. Symfony's request)
  • getLastIpFromList($csv) seems to be only used by getNonProxyIpFromHeader() so same here (and I would make it private).

you can search in all repos not only piwik/piwik

Good point, will do that!

@mnapoli commented on October 29th 2014 Member

@mattab I think I'll leave the plugins call the deprecated code, we can change tehm later when we have solid continuous integration and tests passing (some don't have tests). We will anyway have a way to know when plugins are calling deprecated methods, so I think it's not worth delaying this PR. The sooner it's in the main branch, the more confident we'll be about it.

@mnapoli commented on October 29th 2014 Member

So except that, this PR is good to merge to me now.

@mattab commented on October 30th 2014 Owner

Excellent Pull request :)

we can change them later when we have solid continuous integration and tests passing (some don't have tests)

:+1:

We will anyway have a way to know when plugins are calling deprecated methods

+1 refs #6539

This Pull Request was closed on October 30th 2014
Powered by GitHub Issue Mirror