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.

Sign up for GitHub

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

Jump to bottom

Extract some Piwik Core classes into reusable PHP libraries or reuse 3rd party libraries #6487

Closed
mnapoli opened this issue Oct 21, 2014 · 12 comments
Closed

Extract some Piwik Core classes into reusable PHP libraries or reuse 3rd party libraries #6487

mnapoli opened this issue Oct 21, 2014 · 12 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 21, 2014

This is a topic that has been discussed occasionally, let's open a RFC for that topic.

Piwik Core contains many classes that are not related to analytics. For example: event dispatching, MVC, logging, database, IP helpers, URL helpers, caching, filesystem, unzip… (I could go on)

Our options are:

  1. keep them in Piwik Core, in this repository
  2. extract them into reusable, standalone PHP libraries
  3. replace them with existing 3rd parties PHP libraries

Right now, Piwik is using the option 1 except for a few exceptions (Symfony Console, Twig, Zend_Db). I have also applied solution 2 for the new decompress component.

This RFC is about considering options 2 and 3.

This RFC is not about discussing every component and what should we do with it. It's on a more global level: what do you think of clearing up Piwik Core with options 2 and 3, when applicable?

Pros and cons

Removing code from Piwik Core provides the following benefits:

  • if we can reuse a 3rd party library:
    • less code to maintain
    • easier to learn how to use it (for example many developers know how the Twig works)
    • no need to write and maintain documentation
    • we may attract more contributors (or scare less contributors away) with libraries that everybody uses/knows
  • if we extract a standalone reusable component:
    • we could build several high quality "Piwik components" (a bit like Symfony components or the PHP league of extraordinary packages but of course not covering the same things) and have them as a showcase on Piwik's code quality, and ideally attract new contributors and users
    • much easier to test, higher code coverage, better unit testing (so in theory less bugs)
    • higher quality driven by the need of being reusable
    • much easier for contributors (no Piwik to set up, …)
  • in all cases:
    • clearer codebase
    • less tests to run in Piwik Core (tests runtime is a big problem today)
    • easier for new contributors to understand the whole architecture/code organization and contribute

Cons:

  • if we reuse a 3rd party library:
    • we are exposed to potential bugs in that library
    • we might need to contribute to that library with bugfixes
    • we need to follow up on new versions, make sure nothing broke, etc…
    • we might sometimes need to write adapters, i.e. adapt the API of the library to how we want to use it in Piwik
  • if we extract a standalone reusable component:
    • extra work to keep it standalone and reusable
  • in all cases:
    • we have to care about backward compatibility, generally through adapter classes

When should we use option 1, 2 or 3?

For classes that are not related to Piwik or analytics in general:

  • if we can reuse an existing library that is seriously maintained, then we should reuse it
  • if no 3rd party library does what we want, and if we can provide a better alternative that would be reusable as standalone, then we should open source it as a new standalone component
  • if the classes are too tied to Piwik, then we should keep it in Core

Regarding 3rd party libraries, I would suggest we start with high expectations of quality and reliability. E.g. Symfony components would be OK, we know it is stable, maintained and reliable. For smaller librairies, we have to be careful.

What then?

What do you think of these ideas?

To be clear: this will not be implemented tomorrow. We are just discussing a global direction.
@mnapoli mnapoli added the RFC Indicates the issue is a request for comments where the author is looking for feedback. label Oct 21, 2014
@mnapoli mnapoli changed the title [RFC] Extract some Piwik Core classes into reusable PHP libraries or reuse 3rd party libraries Extract some Piwik Core classes into reusable PHP libraries or reuse 3rd party libraries Oct 21, 2014
@mnapoli mnapoli mentioned this issue Oct 21, 2014
Closed
1 task
@sgiehl
Copy link
Member

sgiehl commented Oct 21, 2014

+1 for extracting some parts into standalone libraries, like we did for component-decompress or device-detector

@mnapoli
Copy link
Contributor Author

mnapoli commented Oct 21, 2014

Ah yes forgot to mention device-detector that's a perfect example! Cool!

@diosmosis
Copy link
Member

Switching to 3rd party libs sounds good for smaller bits of code (like say Piwik\Ip or Piwik\Url). Other than making sure they don't change existing behaviour, we should be sure to examine the libraries carefully. They need to be up to our standards (ie, w/ tests, documentation, etc.). Perhaps each potential move to a 3rd party library could be a separate RFC.

For larger chunks of code that could be replaced w/ a 3rd party lib, I think it would be good to move the code to a standalone repository, but have the code only be a well defined interface layer & default implementation that doesn't rely on a 3rd party lib. Then we can adapt any number of libraries for use w/ Piwik, w/o requiring to use them on the wide variety of environments Piwik must run in.

Finally, regarding creating standalone repos: maybe we should have a list of possible refactorings?

@tsteur
Copy link
Member

tsteur commented Oct 21, 2014

but have the code only be a well defined interface layer & default implementation that doesn't rely on a 3rd party lib

absolutely!

Would prefer single issues for each "refactoring" rather than a list of possible refactorings.

As @diosmosis said we should do this only for some libraries that do not change often etc. Otherwise we'll have a lot of pain with our submodules and/or we need to look for a different solution for submodules

@mnapoli
Copy link
Contributor Author

mnapoli commented Oct 21, 2014

but have the code only be a well defined interface layer & default implementation that doesn't rely on a 3rd party lib

absolutely!

+1 But I don't see why we would need a default implementation if we are using a 3rd party lib. But I definitely agree with having an interface layer (with adapter classes when needed) so that we do not couple Piwik to a specific 3rd party lib.

Perhaps each potential move to a 3rd party library could be a separate RFC.

Would prefer single issues for each "refactoring" rather than a list of possible refactorings.

Definitely agree. I just wanted, before jumping in to specific topics, to make sure we are all on the same page with the global direction. Then we can open issues for each refactoring that we are considering.

we'll have a lot of pain with our submodules and/or we need to look for a different solution for submodules

I don't see how the submodules will be a pain in those cases? We can just install the 3rd party libs with Composer.

@diosmosis
Copy link
Member

But I don't see why we would need a default implementation if we are using a 3rd party lib.

Depends on the specific use case, but since Piwik has to run on so many environments it may be the case that it is not possible for some users to use a 3rd party library. Or such users may not want to use the library/technology because it incurs extra work during setup. In such a case, we would have to provide a default implementation that uses base Piwik technologies (ie, PHP + MySQL). An example of this would be parallel job processing. The default implementations would of course be minimalist.

@tsteur
Copy link
Member

tsteur commented Oct 21, 2014

I don't see how the submodules will be a pain in those cases? We can just install the 3rd party libs with Composer.

I meant only the own libs we create repositories for. 3rd party libs are not a problem.

Didn't see the part about default implementation. Don't think we'll need a default implementation in all cases either. An interface / bridge is enough. Eg for logs, events, ... we wouldn't have to provide a default implementation but the bridge to implement the interface etc.

@mnapoli
Copy link
Contributor Author

mnapoli commented Oct 21, 2014

In such a case, we would have to provide a default implementation that uses base Piwik technologies (ie, PHP + MySQL). An example of this would be parallel job processing. The default implementations would of course be minimalist.

OK I see, I wasn't thinking of such scenarios, it makes total sense. In other simpler scenarios where it's just a simple and plain PHP lib we wouldn't need that though, e.g. what you said @tsteur:

Eg for logs, events, ... we wouldn't have to provide a default implementation but the bridge to implement the interface etc.

@mnapoli
Copy link
Contributor Author

mnapoli commented Oct 21, 2014

I meant only the own libs we create repositories for. 3rd party libs are not a problem.

Ah we would use Composer too for our own libs right? E.g. Piwik uses device-detector or the decompress component with Composer: https://github.com/piwik/piwik/blob/master/composer.json#L40-L41

@diosmosis
Copy link
Member

In other simpler scenarios where it's just a simple and plain PHP lib we wouldn't need that though, e.g. what you said @tsteur:

This makes sense to me.

@mattab
Copy link
Member

mattab commented Oct 22, 2014

This will help keep our technical debt low and keep us agile. 👍

it's a great investment in the future of the platform to create re-usable components. And we should re-use Symfony (or other high quality) components as much as possible, when a similar package is available.

@mnapoli mnapoli mentioned this issue Oct 22, 2014
Closed
@mnapoli
Copy link
Contributor Author

mnapoli commented Oct 22, 2014

Seems everyone is on the same page! We'll have more targeted issues for specific components/classes, let's close this one.

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

No branches or pull requests
5 participants
@diosmosis @tsteur @mattab @mnapoli @sgiehl