I have some code that tightly couples Product specific code and Product-Access code. I am not sure how to untangle it. So for example, for testing Product, I have to mock up Access objects. And maybe there is no way around it, but maybe there is. I have code that goes something like this:
class Product
{
function __construct() {
global $user; //current logged in user object
$id = $user->getId(); //used to make calls to DB
$this->access = $db->query("SELECT access ... where id=$id");
//DB returns "product access" information which is used to allow/reject access
//code of product is tightly coupled with code for authorization
}
}
class ProductA extends Product
{
function __construct() {
parent::__construct();
if ($this->access != "all_access") doPartialAccess();
else doFullAccess();
}
}
//...
class ProductX extends Product
{
function __construct() {
parent::__construct();
if ($this->access != "all_access") doPartialAccess();
else doFullAccess();
}
}
Approach 1
My first thought is to use dependency injection, to inject $user (or $access object directly) into constructor of Product. but wait … I will have to do this with every Product* extending Product. Even if I do this, it will be effectively just moving the instantiation of $user elsewhere in the code (outside of Product* classes). As such, I am not sure it will be the best solution necessarily, because I think it will convolute the codebase with introducing what I think are unnecessary parameters into constructors, instead of making the codebase more maintainable.
Approach 2
Another thought is to have some kind of a “registry” for access object, where I can put objects in it and extract objects out of it. But wait, Registry pattern is deemed an anti-pattern. And I am already using it — see the global $user!? Not an object oriented way to do this, mind you, but registry concept nevertheless.
Question
My long term goal is to decouple product code (various computations, assembling a product model string, etc.) from user permissions code controlling access to the class. Product access constructs are actually used to build some SQL queries, so code of Product and Access are pretty much entangled right now. Product needs Access and I am not sure if I can de-tangle them. I think I can’t walk around the fact that Product does need Access. Perhaps I just need to pull out the currently-sparsely placed code lines that do control access, and put them into their own Access class and call that class ProductAccess, or the like. There may be no way around it, but I am not entirely sure how to do this correctly. DI seems to be inappropriate(?)
My short-term goal is that want to be able to test code for ProductA, without mocking up Access class. But then this could be a wrong goal to have. Maybe I need to mock up Access, since How will I test Product without knowing what level of access it needs…
My overall goal is to find a way to decouple classes as much as I can, while keeping existing functionality and getting rid of the registry pattern I have there now. Is there a way to do this, or is my goal flawed?
3
I think the problem is that your class design is a tad confused, so your question does not have a straightforward answer..
- A product should not have any knowledge of users or access permissions. For instance, an inventory report needs access to all products, independent of which users have access rights.
- You probably should be looking at some form of controller/manager class that does worry about who has access rights to products (perhaps named something like ProductAccessManager). It may ask the product class what user roles are allowed to have access, then check that the requestor is in one of those roles and grant or deny access as appropriate. This separates the access logic from the product itself.
A clean class design avoids the kind of questions you are asking, and generally simplifies testing. I think that some further thought into your class design will pay big dividends in the future.
For this particular problem I would recommend something similar to your’s second aproach.
Check out Service Locator pattern (Is this a good service locator, and is this service locator pattern(?) OK?) this is basically what you called “registry”.
Secondly – don’t put User as a service there. Most probably you need something like Session, and in your code do something like
$user = ServiceLocator::get(“Session”)->currentUser();
I would say ServiceLocator is basically Singleton made mockable 😉
And don’t forget to cover your ServiceLocator with tests 😉
Dependency injection may be useful or not – depending on your framework. For your case it seems that it doesn’t fit – especially that $user is global, so generally it fits Singleton description.