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

Namespace branch and namespace #8

Closed
claytondaley opened this issue Mar 13, 2015 · 8 comments
Closed

Namespace branch and namespace #8

claytondaley opened this issue Mar 13, 2015 · 8 comments

Comments

@claytondaley
Copy link

Can someone add a branch, either:

  • A legacy branch on the assumption that we're using master for namespaced refactoring (or)
  • A develop/namespace branch on the assumption that master will remain the legacy branch

Obviously, the next step is to actually add a namespace. If someone is allowed to make an executive decision on that namespace (and class name), please do so and commit it. Otherwise, what's the process for making such a decision. As a starting point, a step in the right direction is:

Namespace: Piwik
ClassName: PhpTracker

I've also patched together the beginnings of a zf2 tracking module. I was learning ZF2 in parallel so it needs some work, but I would be willing to contribute it to Piwik (under a new name and more permissive license) if there was any interest in heading this direction.

@mnapoli
Copy link

mnapoli commented May 28, 2015

I have created a develop branch, feel free to open pull requests for this branch.

For the namespace, including Php is redundant I think because it's a PHP namespace only. Piwik\Tracker\Client maybe?

@claytondaley
Copy link
Author

I picked "Php" up from the repo name (and intended it mostly to start a conversation), but you're right that it's implied by the time we define a PHP namespace. I suppose there are a couple of points worth making:

  • Several of the Piwik client languages have a separate Tracker and an Api.
  • The most common namespace for existing clients is Piwik\Tracker.
  • The "API" part is less consistent, but often found at Piwik\Analytics

I'd certainly prefer:

  • To see all PHP client code in the same client (which might argue for changing the repo name)
  • A more explicit namespace like Piwik\ApiClient\Tracker (even if we have a separate repo for an analytics client)

I also think it's worth noting that the legacy code provides server-side tracking. It's certainly possible to add the API-supported request for client-side tracking code (as, for example, WP-Piwik currently implements). If we prepare for this possibility, we might use a class more like ServerSide.

All together this would produce the less concise (but more explicit) Piwik\ApiClient\Tracker\ServerSide

@mnapoli
Copy link

mnapoli commented May 29, 2015

I don't understand what you mean by "server side"? The goal here is to use the tracking API, whether it's from a JS client or PHP client doesn't really matter?

@claytondaley
Copy link
Author

Certainly both use the Tracking API. It seems to me that we'd want different classes for the two cases:

  • The existing PHP code that makes calls to the tracker directly
  • PHP code that requests the JS Tracking code (SitesManager.getJavascriptTag) for delivery as part of a webpage (as WP-Piwik does)

I think of this as server-side and client-side tracking because I work mostly in the web analytics use case. Php vs. Js Tracking would be another way to distinguish the two. I'm not sure what the other PHP use cases are to know what else makes sense. Obviously, you could also argue that getJavascriptTag should be put in a ApiClient\SitesManager namespace, preserving all of Tracker for the existing code.

Rather than beg the question... do you agree that these are different classes? If so, what distinction is consistent with the broader set of use cases? I'm not picky... but it'll be really hard to change once there's a base of users so I'm trying to anticipate potential issues.

@mnapoli
Copy link

mnapoli commented May 29, 2015

Getting the JS tracking code is currently not covered by the tracker client, this is probably a problem that can solved later on (wether the method is on the main class or not). It could also be done on the Reporting Piwik client (currently a separate thing, could be merged with this one eventually).

The things we might want to have in a first version could be:

  • namespaces, to avoid polluting the global root namespace and comply with modern practices
  • stateless client, so that it can be used with dependency injection, mocked in tests, etc.
  • use Guzzle or another HTTP client library to avoid re-implementing/maintaining an HTTP implementation when it's not necessary, along with exposing probably more advanced options for some users (they can configure Guzzle however they want), also useful for writing…
  • tests

@claytondaley
Copy link
Author

We seem to be talking past one another. I'm obsessing about namespaces\classnames because they're hard to change after-the-fact.

I'm also 2 steps ahead because the ZF2 module I reference above (and maintain) already includes FQNs [Daley]Piwik\Service\PhpTracker and [Daley]Piwik\Service\JsTracker that implement interfaces like ServerSide... and ClientSide... respectively. I'm already trying to support both tracking strategies and prefer to adopt a Piwik-approved naming convention so the distinction is extremely relevant to me.

Let me break my proposal into pieces so we can identify any points of disagreement:

  1. I'd like to see the whole Piwik API client in one high-level namespace (even if it's delivered in several repos). I'm suggesting Piwik\ApiClient as that overall namespace.
  2. Obviously, it makes sense to use Piwik\ApiClient\Tracker for trackers.
  3. If the JS-based tracker is located in a different namespace, Tracker can be the classname for the existing code.
  4. If we expect to put the JS-based tracker in the same place (my preference), we should use ...\Tracker as the namespace so it doesn't crowd-out the other classname.
  5. If both are placed in ...\Tracker, we also need distinguishing classnames. The Php vs Js and ServerSide vs ClientSide strike me as relevant differences, but am open to anything.

My goal is a Piwik-approved naming convention that covers all of these cases so my ZF2 client stays in sync. Obviously, it also makes sense for me to push code back upstream to this client so it'd be helpful to know where to put JS stuff.

@mnapoli
Copy link

mnapoli commented May 31, 2015

The difference you see in "php tracker" and "js tracker" doesn't exist in this project (yet) :) What I suggested is basically "keep the features, refactor the implementation so that it's more modern" (namespace, DI, tests…). So it doesn't come with any addition of a "js tracker" feature (because it doesn't exist today).

We could open that discussion, but to me it isn't related to namespace discussions. Getting the "Javascript tracking code" is a feature part (today) of the reporting API, not the tracking API. Is it perfect? Dunno, it could change. But after all, this is the PHP client to the tracking API, so unless we decide otherwise this isn't part of this project.

And to be honest, I believe that if we add a way to get the javascript tracking code for a site in this client, then tracking client + reporting client should be merged (but I'm open to be convinced otherwise). My arguments for that are:

  • we would make the limit fuzzy between the 2 clients because we would be querying the 2 separate APIs (tracking and reporting), so we might as well merge
  • getting the javascript tracking code requires a token auth, specific to the reporting API (doesn't exist in the tracking API IIRC)
  • it would be out of the scope of posting tracking data to Piwik (since it would be getting information out of Piwik)

If you want to push this forward and want to start a discussion about merging both API clients, I'm encouraging you to open a discussion on piwik/piwik as it would be a discussion across repositories, so it would make sense IMO to discuss there. Also nobody else is participating in this one so it might be better to have more point of views on the topic.

@mattab
Copy link
Member

mattab commented Dec 27, 2016

Discussion seems outdated. please create new issue if needed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants