SymfonyCasts stands united with the people of Ukraine
gstreamer0.10-ffmpeg
gstreamer0.10-plugins-good packages.
We decided that the confirmation email functionality and user creation functionality are likely to change for different reasons. And so, we split these two responsibilities into two separate classes.
Now, I have some questions. Should we separate the password-hashing logic from the user-persistence responsibility? Meaning, should we move it into its own class? And should we treat the confirmation token generation as its own responsibility and move it somewhere separate?
If you look quickly at SRP, it kinda sounds like the rule is:
Put every tiny piece of functionality into its own class and method.
But, thankfully, SRP is not saying that... that would make our code a disaster! There's another concept called "cohesion". It says:
Keep things together that are related.
At first, it seems like cohesion and SRP are opposites. I mean, SRP says "separate things" and cohesion says "no, keep things together!". But on closer inspection, SRP and cohesion are two ways of saying the same thing: keep only related things together. This is the push-and-pull of SRP: separate things that will change for different reasons... but do not separate any further.
Looking at UserManager, we're already somewhat protected from changes to the password-hashing functionality, because we rely on a service that's behind an interface: UserPasswordEncoderInterface. How that service works could completely change and we wouldn't need to update any code in this class. So the risk of that changing in some way that would cause us to need to change this class is probably very low.
| ... lines 1 - 8 | |
| class UserManager | |
| { | |
| ... lines 11 - 13 | |
| public function __construct(UserPasswordEncoderInterface $passwordEncoder, EntityManagerInterface $entityManager) | |
| { | |
| $this->passwordEncoder = $passwordEncoder; | |
| ... line 17 | |
| } | |
| ... line 19 | |
| public function create(User $user, string $plainPassword): void | |
| { | |
| ... lines 22 - 24 | |
| $user->setPassword( | |
| $this->passwordEncoder->encodePassword($user, $plainPassword) | |
| ); | |
| ... lines 28 - 30 | |
| } | |
| ... lines 32 - 36 | |
| } |
What about the token generation logic? Well, do we think it's very likely that we might change how our tokens are generated? This... to me feels like a weak candidate to separate. It's already simple: one line of code down here... and two lines of code up here. And it's unlikely change, especially for a reason that's different than the other code in this class.
| ... lines 1 - 8 | |
| class UserManager | |
| { | |
| ... lines 11 - 19 | |
| public function create(User $user, string $plainPassword): void | |
| { | |
| $token = $this->createToken(); | |
| $user->setConfirmationToken($token); | |
| ... lines 24 - 30 | |
| } | |
| ... line 32 | |
| private function createToken(): string | |
| { | |
| return rtrim(strtr(base64_encode(random_bytes(32)), '+/', '-_'), '='); | |
| } | |
| } |
Overall, my advice is this: don't over-anticipate potential future changes.
At the beginning of this tutorial, I mentioned a blog post by Dan North, the father of behavior-driven development. He has something delightfully refreshing to say about the single responsibility principle. Instead of thinking about possible changes... and organizing things into responsibilities - which is tricky - he suggests something more straightforward: write simple code.... using the measuring stick of: "does this code fit in my head?".
I love this. If a method or class has too many things in it, then the total logic of that method won't "fit in your head"... and it will be difficult to think about and work with. So, you should separate it into smaller pieces that do fit into your head.
On the other hand, if you split the code for registering a user into 10 different classes, that's also going to become complex to think about. The overall goal is to create units of code that fit in your head... so that you can have an overall application that also "fits in our head".
If you follow this general advice, I think you'll find that you probably create classes and methods that follow SRP pretty nicely... without the stress of trying to perfect it.
Okay, it's time to dive into the next solid principle: the open-closed principle.
Along with what Ryan said, I'd say there are other ways to test private methods.
1) Use reflection to be able to access to the private method
2) Test it indirectly. In this case, you can test what $user->getConfirmationToken() returns
3) Add an integration test for checking the whole process works (create a new user, follow confirmation, link, etc)
I wouldn't test private methods because it means that you test the implementation and not the interface with a black box testing approach. This might lead to fragile tests and bad composition thus encapsulation.
There is only one case that I would test private methods. When I have got a legacy codebase and method is crucial to be tested before the refactor.
So with this manner I would only check exposed, public interface. I encourage you to check https://github.com/sarven/u... there are some nice tips out there
Cheers
Thanks for the link Sargath, it seems useful. One last thing I have to say about testing private methods. I'd rather use reflection or making the method public (and tag it as internal method "do not call this directly") than not testing it at all. Validating your logic works as expected brings much more value than achieving encapsulation
Hey Sargath!
That's a very valid point - thanks for sharing it! We do mention this point later in the OCP section... actually talking about SRP :p - it's at the very end of the chapter - https://symfonycasts.com/sc...
But you're absolutely right. I'm not sure if SRP specifically "concerns itself" about testability. But, in the real world, if I have complex logic, I WILL split it into its own class / public method *just* to be able to unit test it. That, in itself, is another reason for "splitting".
Cheers!
// composer.json
{
"require": {
"php": ">=8.1",
"ext-ctype": "*",
"ext-iconv": "*",
"composer/package-versions-deprecated": "^1.11", // 1.11.99.1
"doctrine/annotations": "^1.0", // 1.12.1
"doctrine/doctrine-bundle": "^2", // 2.3.1
"doctrine/doctrine-migrations-bundle": "^3", // 3.1.1
"doctrine/orm": "^2", // 2.8.4
"knplabs/knp-time-bundle": "^1.15", // v1.16.0
"phpdocumentor/reflection-docblock": "^5.2", // 5.2.2
"sensio/framework-extra-bundle": "^6.0", // v6.1.2
"symfony/console": "5.2.*", // v5.2.6
"symfony/dotenv": "5.2.*", // v5.2.4
"symfony/flex": "^1.9", // v1.18.7
"symfony/form": "5.2.*", // v5.2.6
"symfony/framework-bundle": "5.2.*", // v5.2.6
"symfony/http-client": "5.2.*", // v5.2.6
"symfony/mailer": "5.2.*", // v5.2.6
"symfony/property-access": "5.2.*", // v5.2.4
"symfony/property-info": "5.2.*", // v5.2.4
"symfony/security-bundle": "5.2.*", // v5.2.6
"symfony/serializer": "5.2.*", // v5.2.4
"symfony/twig-bundle": "5.2.*", // v5.2.4
"symfony/validator": "5.2.*", // v5.2.6
"symfony/webpack-encore-bundle": "^1.6", // v1.11.1
"symfony/yaml": "5.2.*", // v5.2.5
"twig/cssinliner-extra": "^3.3", // v3.3.0
"twig/extra-bundle": "^2.12|^3.0", // v3.3.0
"twig/twig": "^2.12|^3.0" // v3.3.0
},
"require-dev": {
"doctrine/doctrine-fixtures-bundle": "^3.2", // 3.4.0
"fakerphp/faker": "^1.13", // v1.14.1
"symfony/debug-bundle": "^5.2", // v5.2.4
"symfony/maker-bundle": "^1.13", // v1.30.2
"symfony/monolog-bundle": "^3.0", // v3.7.0
"symfony/stopwatch": "^5.2", // v5.2.4
"symfony/var-dumper": "^5.2", // v5.2.6
"symfony/web-profiler-bundle": "^5.2" // v5.2.6
}
}
There is also important to remember about the tests. With this implementation we can only test public methods of `UserManager` (for instance, the Boston way input-output). In such case we cannot unit test token generation in separation. So I would slightly argue about some concepts presented here ;)
Cheers