By Kore Nordmann, first published at Tue, 10 Jan 2017 09:34:30 +0100
Download our free e-book "Crafting Quality Software" with a selection of the finest blog posts as PDF or EPub.
You can also buy a printed version of the book on Amazon or on epubli.
staticWhen people start (unit-)testing their code one of the worst problems to tackle are static calls. How can we refactor static calls out of an existing application without breaking the code and while producing new features? How can we get rid of this big test impediment?
Illustrating the problem with example code is only partially possible – the sheer amount of static calls found in real-world software is way too large. The problem is worse by at least a magnitude from everything you'll see in this blog post. But here goes an example anyways:
class UserService {
/* … */
public static function getUser($userId) {
$cacheKey = 'user-' . $userId;
if (Cache::has($cacheKey)) {
return Cache::get($cacheKey);
}
$userData = DB::query('SELECT * FROM user WHERE id = :id', ['id' => $userId]);
$user = new User($userData);
Cache::set($cacheKey, $user);
return $user;
}
/* … */
}Using the cache statically is only one common thing to do. Usually it is also the logger, the database and about any service. Probably it is even the UserService itself with calls like UserService::getUser(42) spread all over your code.
Today it is an established best-practice that we should test our code. People working with code like the one shown above know this and want to apply automated testing. It is also clear that the most desired testing method are unit tests. When writing new methods in the UserService: How can we test those? Or how can we test the existing methods in the UserService?
The core problem when testing the UserService is that we cannot mock the Cache instance for testing. The original Cache class will always be called, thus we will always test the Cache class together with the UserService. This is, by definition, not an Unit Test any more. Depending on the used cache implementation this might also require a far more complex setup to run the tests. If the cache implementation directly caches into Redis for example, you need a working Redis server just to run the UserService tests.
But getting away from static calls isn't a thing you should do in a single refactoring step. In fact, migration must be smooth and longer running to align with business perspectives. To achieve this, we show you multiple steps in such a migration.
The workaround for the primary testing problem is obvious and employed by many developers – you can even find complete testing frameworks build on this approach, like the one from Laravel: Make the implementation behind the static calls replaceable. Laravel calls this "Facade", while the Facade Pattern actually is something different.
The idea is that the Cache class gets a setter for its actually used cache implementation, like:
class Cache {
private static $implementation;
public static function setImplementation(CacheImplementation $cache) {
self::$implementation = $cache;
}
/* … */
}In your test case you can now use something like this:
class UserServiceTest extends \PHPUnit_Framework_TestCase {
public function testLoadUser() {
// Could happen in setUp()
$originalCache = Cache::getImplementation();
/* Set up mock */
Cache::setImplementation($cacheMock);
/* Actual test setup */
/* Stimulus */
/* Assertion */
// Reset should happen in tearDown()
Cache::setImplementation($originalCache);
}
}Why is this still problematic?
Global side effects
The worst thing here is the global side effect of this change. Since $implementation has to be a static variable in the Cache class it is also changed for any other code, like future tests. To get atomicity of your tests you must also reset the cache implementation after the test again.
More complex test setup
If you would use dependency injection the test code would only consist of the commented out code and would not require the setting and re-setting of the cache implementation. This might look trivial in this code but there are usually more classes used.
Also tests should focus on readable code even more then any other code since they are often the starting point to understanding code for other developers. Anything messing unnecessarily with the test code can be considered bad.
API differences due to indirection
The APIs of the Cache class and the CacheImplementation class might be different which means that you have to mock the internal usage inside of the Cache class and not the usage inside of the UserService class. You must at least ensure that those APIs are the same. This means that your brain must keep more context which leaves less focus on the actual implementation and test.
This is a valid workaround introducing additional complexity because of global state and unnecessary indirection. The static calls allow you to skip dependency injection by introducing additional complexity while understanding the code and while testing the code. A trade-off I am not willing to accept as a migration step but not in the long run.
Looking at the actual target of our refactorings, the UserService we desire looks like this:
class UserService {
private $cache;
private $database;
public funtion __construct(CacheImplementation $cache, Database $database) {
$this->cache = $cache;
$this->database = $database;
}
/* … */
public function getUser($userId) {
$cacheKey = 'user-' . $userId;
if ($this->cache->has($cacheKey)) {
return $this->cache->get($cacheKey);
}
$userData = $this->database->query('SELECT * FROM user WHERE id = :id', ['id' => $userId]);
$user = new User($userData);
$this->cache->set($cacheKey, $user);
return $user;
}
/* … */
}What changed? We migrated all static dependencies to dependency injection via Constructor Injection and removed the static keyword for the getUser() method (and all others).
This code is testable. We can inject mocks for the CacheImplementation and Database directly and will not have any global side effects affecting out test atomicity. There is no environment related test setup required any more. The method itself could be cleaner and easier to test, but this is not the point right now. One important remark: You can also call every static method
dynamically. Thus we can just pass an instance of Cache or CacheImplementation and Database and there should not be any problems.
The problem is that the API of our UserService changed. If we had code earlier calling UserService::getUser(42) it will not work any more. A simple step to get this code working is introducing a static Service Locator as a workaround during refactoring:
class ServiceLocator {
private static $userService = null;
/* Same for cache(), database(), … */
public static function userService() {
if (!self::$userService) {
self::$userService = new UserService(self::cache(), self::database());
}
return self::$userService;
}
}The we need to migrate all calling code to call ServiceLocator::userService()->getUser(42) instead of UserService::getUser(42). This refactoring is easy enough and can usually be done by Search & Replace: s/UserService::/ServiceLocator::userService()->/g
The code using the UserService is still not clean, but we cleaned up the UserService itself to use dependency injection and be testable. We are not done yet, but this already is a big step forward.
Until now all steps are quick to accomplish, but the next one will take time.
Now we have the UserService in the desired state. We already changed the code using it to indirect static calls through the ServiceLocator. But we should eventually get rid of all the static access, including the static access to the ServiceLocator itself.
Problems with using a Service Locator are:
Still global side effects (when used statically)
If you test a class which itself accesses the Service Locator you have to replace all requested services with mock objects in the Service Locator. Since the Service Locator implementation uses static state it will still affect all future tests (without proper resetting logic). This breaks test atomicity again.
…, thus also: Complex test setup
Hidden dependencies
If a class has access to the Service Locator you can only understand its dependencies from reading its entire code. In a class using Constructor Injection we know all its (required) dependencies by just looking at the constructor signature.
A class which has access to the Service Locator can request any class anywhere in its code from it. How do we know what to mock in a test case or what setup to create if we want to use the class somewhere else? You'll have to read the entire code.
This is also true for Service Locators with dynamic access which are passed around. They can be another intermediate refactoring step but usually provides nothing of additional value.
Consequence: Pass the ``UserService`` instance to every class which needs access to it. Or phrased differently: Use Dependency Injection for all your code – or at least all code which is supposed to be tested or re-used at some point.
This means that, in production, you'll have just one instance of the UserService which is passed to any class which needs to access the users. This seems like a lot of work and it really is. But most of the work can actually be taken over by even the simplest Dependency Injection Container:
class DependencyInjectionContainer {
private $userService = null;
/* Same for cache(), database(), … */
public function userService($dic) {
if (!$this->userService) {
$this->userService = new UserService($this->cache(), $this->database());
}
return $this->userService;
}
public function userController($dic) {
return new UserController($this->userService());
}
}OK, this looks really similar to the ServiceLocator class shown before – only dropping all static. This is right. The difference between a Service Locator and a Dependency Injection Container is not the implementation but the usage:
The DependencyInjectionContainer is only used inside your index.php and not passed to any class. As an additional migration step you may use it inside your Service Locator until we migrated away from it entirely.
If you have a simple routing definition like with Silex you should use the Dependency Injection Container right there – and nowhere else:
$app = new Silex\Application();
$dic = new DependencyInjectionContainer();
$app->get('/user/{id}', function ($id) use ($app, $dic) {
return $dic->userController()->getAction($id);
});
$app->run();The Dependency Injection Container will now resolve all the required dependencies only when the route is called. You can even replace the code above by using simple Dependency Injection Containers like Pimple.
This is the last step to have only testable classes all using Dependency Injection. The last step is a lot of work because many classes must be adapted. But you achieved actually two goals by doing this:
Make everything testable
Extract application configuration
Which cache is used by the user service should not be the concern of the user service itself but is nothing but application configuration. Now the DependencyInjectionContainer contains all your application configuration (it may access parameter files or similar) and you configure the used cache implementation there – and it will be "magically" used everywhere.
We can help you refactoring your existing code while keeping your application functional. We will ensure through workshops, refactorings and code reviews that your goals are met. Hire us for a kick-off workshop.
Migrating away from static calls can be quite some work. This blog post showed you a migration strategy with functional software in every step. Consider static a code smell because of all the reasons mentioned in this post and migrate away from it eventually. Take your time.
Stay up to date with regular new technological insights by subscribing to our newsletter. We will send you articles to improve your developments skills.
Baileylo on Tue, 10 Jan 2017 11:46:35 +0100
Your second example of Service Locator is closer to what laravel does than using a set methods. The facades, reference a specific item out of the service locator.
Link to commentYou also bring up that testing static methods is a pain and at least requires a large amount of work. I'd argue that using facades does not incur this extra effort or any extra difficulties. But will whole heartedly agree that most static methods are more difficult to test.
Rasmus Schultz on Wed, 18 Jan 2017 08:16:41 +0100
Your examples are great, but what this article lacks is examples of when you should use static - it comes off as another campaign against static, which, frankly, we've had enough of in the past.
Link to commentStatic is not "evil" - in fact, given php's crippling inability to autoload functions, it's necessary. The rule is simple: static methods must not rely on state. They must be pure functions that behave precisely the same every time for any given input, must not have any side-effects or external dependencies, etc.
For lack of better, I tend to designate (in a class-level doc-block) some classes as a "pseudo-namespace" and declare it as abstract to prevent instanciation. A collection of flat functions that happen to reside in a class declaration are perfectly testable - flat functions are terribly underused in PHP and code tends to be much more "object oriented" than necessary, I think, mainly because functions are essentially useless in PHP, and a pseudo-namespace is currently your best option; avoiding the use flat functions entirely is not a healthy solution, as functions are often much better suited to a problem, and much simpler to deal with than objects.