Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extracting the IP class into a standalone "network" component #6494

Closed
mnapoli opened this issue Oct 22, 2014 · 4 comments
Closed

Extracting the IP class into a standalone "network" component #6494

mnapoli opened this issue Oct 22, 2014 · 4 comments
Labels
RFC Indicates the issue is a request for comments where the author is looking for feedback.

Comments

@mnapoli
Copy link
Contributor

mnapoli commented Oct 22, 2014

Related to #6487

What about moving the Piwik\IP class out of Piwik and into a standalone repository?

Most of this class is completely unrelated to Piwik, I've done a POC and it was fairly easy to move it out. The good thing with this is that we can take advantage of this to refactor it into a non-static class. And we can also add more unit tests as the new class will have no global state.

Piwik\IP would actually be kept where it is for backward compatibility, however the content of the class would be moved.

After a quick discussion with some of us we thought naming that new component network would be better than ip, it leaves us the opportunity to move additional classes in there later if applicable. Thoughts?

We also thought about moving Piwik\Url in the new component. But I disagree with that because Piwik\Url has actually much more to do with the current request than urls (i.e. it's mostly a wrapper over $_SERVER). I see that class replaceable with a Request class (either Symfony or our own or whatever, that's not the topic), and to me it belongs more in a HTTP/MVC component (like Symfony's HttpFoundation for example). For the few methods that are strictly related to URLs (i.e. manipulating string urls), 3rd party librairies already cover that (e.g. http://url.thephpleague.com/). So all in all, I don't think Url should be moved to the network component, at least not right now.

@mnapoli mnapoli added the RFC Indicates the issue is a request for comments where the author is looking for feedback. label Oct 22, 2014
@mnapoli
Copy link
Contributor Author

mnapoli commented Oct 22, 2014

There's also something really cool: we can move the IP anonymization stuff from the IPAnonymizer plugin into the new IP class. Anonymizing an IP address is pretty standard stuff (for a library to manipulate IPs) so I think it definitely belongs there. That's also good because we can move some more tests out of Piwik.

@diosmosis
Copy link
Member

👍 for everything here

@mattab
Copy link
Member

mattab commented Oct 22, 2014

👍 it would be great to move IP + IP anonymise out of core into its own component

@mnapoli
Copy link
Contributor Author

mnapoli commented Oct 23, 2014

I have created a pull request here: #6517

@mnapoli mnapoli closed this as completed Oct 23, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RFC Indicates the issue is a request for comments where the author is looking for feedback.
Projects
None yet
Development

No branches or pull requests

3 participants