@mnapoli opened this Pull Request on August 7th 2014 Contributor

This is a POC of the refactoring to get rid of singletons and static classes. This is the step 1 described in #4917.

  • Piwik\Db was a static class, i.e. all its methods were static.

I added Piwik\Db\Db (do you like the name? would Database be better?) and moved the code of Db into it. This new class is not static, and uses dependency injection.

Piwik\Db is now a static proxy to Piwik\Db\Db. So for example calling Db::fetchAll(...) will be forwarded to self::$db->fetchAll(...) (Db\Db).

We can now inject the non-singleton (Db\Db) into whatever (in theory). But before doing that, we'll need to move the creation of that object into a container (right now it's created in Db\getInstance()).

We can now also unit tests Db\Db without hitting any database, because we can pass a mock instance of the DbAdapter. Yay.

  • Piwik\Access was a singleton.

I added Piwik\Authorization\Authorization and now Piwik\Access just forwards to this new class.

I think it was probably a mistake to do that because it's not necessary, I just got carried away and thought the new name was more explicit (I didn't get what "Access" meant at first, authorization is more standard). But whatever no big deal, we can revert that.

So a better solution would be probably to have Access extend Authorization (if we want to keep that name and namespace) and it would just add the singleton access for BC compatibility. That would avoid useless call forwarding.

If we don't want to rename it, then we can just keep Access as it is and then when we have a container just have the getInstance() call the container instead of creating the object directly.

@czolnowski commented on August 7th 2014 Contributor

+1 for Db\Database class
and +1 for Authorization class (more standard name than Access what for me mean like a special case of Authorization)

@mnapoli commented on August 7th 2014 Contributor

I have changed Access: it now extends Authorization which make much more sense, and it avoids lots of useless code.

Edit: the tests seem to be running OK now, I think the ones that are failing are just because master is failing too.

@mnapoli commented on October 5th 2014 Contributor

Closing this as this was a POC.

This Pull Request was closed on October 5th 2014
Powered by GitHub Issue Mirror