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

Organisation of \Piwik\Plugin\* classes #7929

Closed
tsteur opened this issue May 18, 2015 · 7 comments
Closed

Organisation of \Piwik\Plugin\* classes #7929

tsteur opened this issue May 18, 2015 · 7 comments
Labels
not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. RFC Indicates the issue is a request for comments where the author is looking for feedback.
Milestone

Comments

@tsteur
Copy link
Member

tsteur commented May 18, 2015

We currently have a couple of classes under Piwik\Plugin\* classes. Eg Piwik\Plugin\Report, Piwik\Plugin\Widget, Piwik\Plugin\Controller. We put there classes that can be extended by plugins to create new entities, eg Widgets, Reports, ...

It is quite nice to have all those classes under Piwik\Plugin and to see at a glance which classes can be extended and what kinda our main API's are. Though I'm not so sure if it's still the right way.

Eg where do we put Widget related classes such as WidgetFactory or WidgetsList. Under Piwik\Plugin\Widget\Factory and Piwik\Plugin\Widget\WidgetsList?

Or should we move Piwik\Plugin\Widget to Piwik\Widget and Piwik\Widget\Factory and Piwik\Widget\WidgetsList? Or mixed?

We currently also have Piwik\Db, Piwik\Url, Piwik\DataTable and I reckon for plugin developers and for us it's maybe not clear where to find/put which class.

Any opinions / thoughts?

With Piwik 3.0 we could break the API and maybe move those classes although it could be quite a bit of work, especially to also keep all of our plugins compatible.

@tsteur tsteur added the RFC Indicates the issue is a request for comments where the author is looking for feedback. label May 18, 2015
@tsteur tsteur added this to the 3.0.0 milestone May 18, 2015
@dczajkacc
Copy link

I would keep related classes together and in logical namespaces, e.g.:

Piwik\Widget\List - former WidgetsList
Piwik\Widget\Factory - former WidgetFactory
Piwik\Widget\Provider - former Widgets

@mgazdzik
Copy link
Contributor

I think it would also make sense to distribute this by domain, as @dczajkacc wrote.

@mnapoli
Copy link
Contributor

mnapoli commented May 19, 2015

There's one thing I dislike with the Piwik\Plugin\ namespace is that it's very easy to confuse with Piwik\Plugins\.

I would be +1 to move related things together as suggested, e.g. Piwik\Widget\..., Piwik\Report\..., etc.

We currently also have Piwik\Db, Piwik\Url, Piwik\DataTable…

I honestly hate things in the root namespace ;) I would definitely be +1 to reorganize things in separate namespaces (e.g. Piwik\Database\…). Especially since we have the old ZF1/PEAR convention with Piwik\Db in the root namespace and the related classes in a subnamespace (Piwik\Db\AdapterInterface, …). (keeping related things together is better)

However there's one advantage to keep the current [not ideal] names: it allows us to refactor the component later and use the good name without breaking BC. For example with the scheduler the old class was Piwik\TaskScheduler (IIRC) and the new refactored class was Piwik\TaskScheduler\TaskScheduler. That allows to keep the old class with the old name as an adapter to the new class, which is very cool because then we can write the new class however we want.

With Piwik 3.0 we could break the API and maybe move those classes although it could be quite a bit of work, especially to also keep all of our plugins compatible.

We can also set up class aliases or write subclasses to keep BC.

@tsteur
Copy link
Member Author

tsteur commented May 19, 2015

I honestly hate things in the root namespace ;)

That's a strong opinion :) I don't mind them there and find it can be even nice sometimes. I'd rather find Piwik\Date\Date or so not as nice when a shorter version (Piwik\Date) does it as well. A problem that I see with those classes is that they sometimes contain too many things and too many different things. Also there are some classes that rather belong into a different namespace maybe such as Piwik\Cookie could be under something like Piwik\Http etc. Saying there are cases where it's nice to have something under the root namespace but it's also nice to have all in one namespace.

How would we rename eg Piwik\Plugin\Widget under the Widget namespace? One suggestion was Piwik\Widget\Provider or basically Piwik\Widget\WidgetProvider. Any other ideas?

@mnapoli
Copy link
Contributor

mnapoli commented May 20, 2015

Hate maybe was a strong word rather "dislike" ;)

WidgetProvider could do but are we talking about the classes that plugins extend to define widgets? If yes then either each implementation is a widget (in which case the natural name for the class would be Piwik\Widget\Widget I'd say), or either each implement provides widgets (plural) (in which case Piwik\Widget\WidgetProvider makes sense)?

@tsteur
Copy link
Member Author

tsteur commented Jun 3, 2015

For now I put everything under Piwik\Widget. Eg Piwik\Widget\Widget, Piwik\Widget\WidgetConfig etc

@tsteur tsteur closed this as completed Jun 3, 2015
@tsteur
Copy link
Member Author

tsteur commented Jun 3, 2015

Feel free to comment etc if someone has new or more thoughts on this

@mattab mattab added the not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. label Jun 24, 2015
@mattab mattab modified the milestones: 3.0.0, 3.0.0-b1 Jul 30, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. 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

5 participants