[WIP] Refactoring

[ThaDafinser]

Hello,

since this is a BC break anyway i would like to use it as a refactoring point.

• namespacing
• cleanup / splitting
• phpunit testing
• ...

I started here https://github.com/ThaDafinser/piwik-tracker

[mattab]

I think it's important that we do not break backward compatibility for this class: Piwik users expect us to maintain Backward Compatibility. I'm not sure what is best course of action... could we maybe just migrate to composer, without changing class, and then use a bridge in https://github.com/piwik/piwik/blob/master/libs/PiwikTracker/PiwikTracker.php to load the class from vendor/ ?

[mattab]

Hi @ThaDafinser - are you still interested in this project?

If not, no worries, we could maybe just create an issue on Github tracker: https://github.com/piwik/piwik/issues - and close this repository in the meantime. Let me know :)

[tsteur]

+1 for having PiwikTracker in a separate repository and using namespaces. Using composer makes sense as well, you do not want to manually copy/paste the PiwikTracker from a Piwik build into your PHP project after each Piwik update. Makes it also easier to maintain and contribute for developers by having a separate repository for this. We could simply recommend people to use this one from now on (update docs, mention it in next release blog post, introduce project in a separate blog post). As long as we keep the public API of the tracker class the same people would only have to import the namespace to migrate. Would give us also the chance to refactor the internals of the Tracker and to make it testable.

We could mark the old one as deprecated from now on and remove it for instance with Piwik 3.0. The old one would basically only receive security fixes and important bug fixes which shouldn't be too much work for us. So it would be still backwards compatible.

[ThaDafinser]

@mattab yep still.

But i'm also still on +1 the refacotring like @tsteur.

I think this is a great chance to refactor, wihout breaking existing code (since it's a new repo...).
Like i started here: https://github.com/ThaDafinser/piwik-tracker

I think a good way to keep BC for some time is:

• refactor in the new repo
  • composer
  • namespaces
  • better splitting / cleanup
  • unit tests / CI / ..
• keep the original tracking code in the piwik main repo for additional 1-2 versions from the point on the refactored repo is stable

[bretrzaun]

+1
If the current state of the Piwik Tracker would be published as a 1.0 release, people can start using this version in existing projects.
In following major versions refactorings can take place.

[cupracer]

+1 for @bretrzaun
I'd really appreciate having the current state of the Piwik Tracker published here.
Thanks!

[ThaDafinser]

Ok my final suggestion, based on the feedback:

• copy unmodified the original class https://github.com/piwik/piwik/blob/master/libs/PiwikTracker/PiwikTracker.php
• migrate to namespace PiwikTracker without php
  • https://github.com/piwik/piwik-java-tracking/blob/master/src/main/java/org/piwik/PiwikTracker.java
  • https://github.com/piwik/piwik-python-api/blob/master/piwikapi/tracking.py
• leave root classname PiwikTracker
  • use statename would be then: use PiwikTracker\PiwikTracker;
• move functions out: https://github.com/piwik/piwik/blob/master/libs/PiwikTracker/PiwikTracker.php#L1696-L1722
  • create PiwikTracker\Functions.php and let it "autoload" by composer
    • https://getcomposer.org/doc/04-schema.md#files
• create PiwikTracker\TrackerException to extend standard Exception
• use trigger_error for deprecated things? http://php.net/manual/de/function.trigger-error.php
• add unitTests
• integrate Travis CI
• create composer.json
• update Readme.md
• release it on packagist

The only BC break is then, to adope the new namespace and since people have to switch to composer on the first time, this can be done in one step.

@mattab @tsteur any other things left (other naming), or can i start?

[mnapoli]

Great stuff!

If I understood correctly there are 2 problems with BC compatibility:

• switching to Composer
• namespacing the class

For the second problem, there are 2 solutions. If you want to break the API of the class to refactor it in subclasses and all, then you can put a \PiwikTracker class in this package as an adapter of the new class (it would just forward calls to the new class). That way migrating to the new package for users is just about using Composer, people don't even have to care about the namespace change.

If you don't need to break the API of the class, then you could simply do a class_alias() so that PiwikTracker still exists but is an alias of the new namespaced class.

Either way, that would keep BC compatibility for the code, people would just have to switch to Composer (or download the new package manually).

Regarding the namespace, I would strongly suggest to follow PSR-0 and keep the standard pattern of Vendor\Package\...: Piwik\Tracker\... would be more consistent with the current namespacing in piwik/piwik. And it would be also more consistent if parts of Piwik's core are extracted into smaller subprojects like this one in the future.

[claytondaley]

I appreciate that there's some legitimate discussion in here. I actually found this repo when trying to pull the PHP tracker into ZF2 (so I get the namespace questions). To be blunt, I'd just benefit from having a repo to grab the tracker (without additional maintenance complexity). If it supports composer and git submodule, all the better. In any case, please don't let perfect be the enemy of useful.

[claytondaley]

I'd even vote for a clean split. Leave a legacy repo (or branch) with the old PiwikTracker and start a new Piwik\Tracker repo (branch). They can sit side-by-side in the Piwik libs directory for all I care.

The PHP Tracker has lagged (and likely still lags) behind the API features in the core. I should know since I've contributed several missing features. Adding an additional burden of backwards compatibility seems like a waste of what attention PHP tracker gets. At the same time, abandoning a legacy codebase that is already lagging behind (except, perhaps, for bugs and security issues) seems like a rounding error for these users.

[piotr-cz]

I'd be for clean split (no extra features as it's presumably 1.0).

No tests to speed things up, just change just in namespace and added composer.json

[tsteur]

@claytondaley good point! I will move the existing tracker into this repository and add a composer.json. In Piwik we will include this repository as a submodule for now. If anyone wants to start a refactoring in a branch feel free to do so! Not sure if I will find the time to work on it soon

[ThaDafinser]

👎 for only moving, but seems that many people wants that.

Reason: this would be a good time for BC breaks, which are much harder laterr. After this repo is released with composer we have users and need to be BC compatibel.

But i'm also fine with this, if more people wants it that way. 👍

For rewrite i'm still open...

[piotr-cz]

@ThaDafinser Appreciate to good part: this is a good start. People can start including PhpPtracker in their projects now and there may be BC progress in development without affecting usage of v1.x.

BTW: I'll be happy if the library would use SemVer.

Also, what's the advantage of using library as a submodule in Piwik against using Composer depedency? I mean, there are other 4 Piwik repos included in composer.json.

[mattab]

I think this is the beauty of this solution: we could even keep BC forever by leaving the submodule point to the old 1.X tracker. then we can work on 2.0.0 release with namespace and composer users can use it and benefit from it, while we keep BC for all existing users who dont want to never have to change their code (understandable)

[tsteur]

Exactly. We can add another class including namespaces which doesn't have to stay backwards compatible and include this package via composer in Piwik itself but also in other projects. We might not keep BC for the old tracker forever but for quite a while for sure. That's the idea. If someone wants to start a refactoring feel free to do so.

We included this repository as a submodule in piwik/piwik to keep BC in Piwik as some projects might expect the PiwikTracker under the libs directory. We could use another composer package to move only this package to the libs directory later.

[julienmoumne]

How about not having to maintain a PHP version anymore ? matomo-org/piwik-dotnet-tracker#12

[mattab]

We're now managing the project with composer and we kept full backward compatibility. If anyone wants to help further improve the project please open new pull request / issue 👍 You'll be very welcome 😄