PHP story with code examples, copy-cat killers, just a little bit of static, consistency, sniffs and way to prevent all that ever happening ever again.
Today the format will be reversed - first I'll show you practical code and its journey to legacy, then theory takeaways that would save it.
Symplify\CodingStandard contains complex Sniff and Fixers like the doc block cleaner. Job of RemoveUselessDocBlockFixer is clear - remove any doc block that has no extra value over the php code itself:
/**
- * @param int $value value instance
- * @param $anotherValue
- * @param SomeType $someService A SomeType instance
- * @return array
*/
public function setCount(int $value, $anotherValue, SomeType $someService): array
{
}
The goal is clear, but how does it work beneath the surface? There are multiple steps that Fixer needs to perform one by one:
Is that all? Nope. There also code that handles php docs:
Just a reminder, this all started with a simple idea:
-/**
- * @param int $value
- */
public function compute(int $value)
{
}
Today I'll write about how code always grows and that we should anticipate it and code the best way we know right from the beginning. And what happened to me when I thought I could handle it by using static methods only where it makes sense (well, everything makes sense, until it's legacy drowning you down).
Let's look how the fixer grew to the point it turned into legacy, how that shot myself and what I could (and will) do better to prevent it.
(To skip irrelevant details, I'll use pseudocode instead of original full code.)
class RemoveUselessDocBlockFixer
{
public function fix($tokens)
{
foreach ($tokens as $token) {
if (! $token->isMethod()) {
continue;
}
// it's a method!
$docBlock = $this->getDocBlockByMethod($token);
if ($docBlock === null) {
continue;
}
// it has a doc block!
$this->removeUselessContentFromDocBlock($docBlock);
}
}
}
That's a basic workflow. In Easy Coding Standard 3 and below, checkers have no constructor injection, only new and ::static methods were allowed. I took this inspiration from PHP CS Fixer where new is a first class citizen. There is no DI container, just static instantiations. Maybe that should warn me, but I said to myself "it's a popular package, it has new fixers from time to time and it's tagged once a while, it must be good and they know what they're doing".
So back to the code:
public static function getDocBlockByMethod($token)
{
$docBlockPosition = DocBlockFinder::find($token);
if ($docBlockPosition === null) {
return null;
}
return DocBlockFactory::createFromPosition($docBlockPosition);
}
public static function removeUselessContentFromDocBlock($docBlock)
{
DocBlockCleaner::processParamAnnotations($docBlock);
DocBlockCleaner::processReturnAnnotations($docBlock);
}
You see where it goes. The biggest potential black hole is always 3rd party code (unless it's your code). I could write the docblock parser myself or make use of phpDocumentor/ReflectionDocBlock. It was the best on the market in that time. Neither ready for PHP 5.5+ features like variadics nor formatter preserving printer. Except that it worked pretty well.
class DocBlockFactory
{
public static function createFromPosition($docBlockPosition)
{
$tagFactory = new StandardTagFactory($fqsenResolver, [
'param' => TolerantParam::class, // own overloaded class
'return' => TolerantReturn::class, // own overloaded class
'var' => Var_::class, // own overloaded class
]);
$descriptionFactory = new DescriptionFactory($tagFactory);
$tagFactory->addService($descriptionFactory);
$tagFactory->addService(new TypeResolver($fqsenResolver));
$phpDocumentorDocBlockFactory = new DocBlockFactory($descriptionFactory, $tagFactory);
return $phpDocumentorDocBlockFactory->create($docBlockPosition);
}
}
So every time a single doc block is created, more than 10 classes (counting these on background) are created too. It might be a small deal for performance, but even bigger for legacy code smell that might hit me back later. But whatever, YOLO!
And here all the static fun ends. Well, not yet, because it worked. I talked a lot with the maintainer of phpDocumentor/ReflectionDocBlock about moving it forward, but as I was the only one trying, it didn't lead much further than issue chats and PRs that were opened for too long. It was only logical that without monorepo all the time was swallowed only by maintenance of 4 interdependent packages.
Then Jan Tvrdík came with a support package for PHPStan for handling php docs - phpstan/phpdoc-parser. It is built on similar principles as nikic/php-parser, much younger and robust.
I thought: "I'd like to try that one package in my code", but how?
It's easy, just replace all the old static classes with new ones:
class DocBlockFactory
{
public static function createFromPosition($docBlockPosition)
{
$content = $this->getContentOnPosition($docBlockPosition);
$lexer = new Lexer;
$tokenIterator = new TokenIterator($lexer->tokenize($content));
$phpStanPhpDocParser = new PhpStanPhpDocParser(new SomeDependency(new AnotherDependency));
return $phpStanPhpDocParser->parse($tokenIterator);
}
}
Do you need to add whitespace config? Just add it in every layer... or make it also static.
class DocBlockFactory
{
public static function createFromPosition($docBlockPosition)
{
$content = $this->getContentOnPosition($docBlockPosition);
$lexer = new Lexer;
$tokenIterator = new TokenIterator($lexer->tokenize($content));
$phpStanPhpDocParser = new PhpStanPhpDocParser(new SomeDependency(new NewAnotherDependency));
- return $phpStanPhpDocParser->parse($tokenIterator);
+ $docBlock = $phpStanPhpDocParser->parse($tokenIterator);
+ $docBlock->addWhitespaceConfig($this->whitespaceConfig);
+
+ return $docBlock;
}
+
+ public function setWhitespaceConfig(WhitespaceConfig $whitespaceConfig)
+ {
+ $this->whitespaceConfig = $whitespaceConfig;
+ }
}
But what if you forget to add it
+ public function ensureWhitespaceConfigIsSet()
+ {
+ if ($this->whitespaceConfig) {
+ return;
+ }
+ throw new WhitespaceConfigNotSetException(sprintf('Informative message in "%s" method', __METHOD__));
+ }
Congratulations, you've just made a static container all over your code, similar to Laravel Facades. Uff, I just get headache by writing this code.
But why stopping there? Let's add a configuration that will tell the DocBlockFactory if the starting tag should be /* or /**.
Well, shoot me now!
Dependency injection First. Not first, but only dependency injection.
I told myself - "here the static method makes sense, it's just one little method". The problem is, that static methods work well only with other static methods. You simply can't inject a service to a class with static methods and use it statically.. well, to be honest, Laravel did it in facades and Reflections, but you should not. Unless you want to use such approach in the whole codebase. That would be the only valid reason to do it so.
So be consistent in architecture pattern you pick.
It took me 3 pull requests to get out of this mess. Not to try the new package, just to prepare the code to be able to do so. Instead, I could have a clear DI design, use one PR at a time to trying this package and other 2 PRs could have been new features.
A copycat crime is a criminal act that is modeled or inspired by a previous crime that has been reported in the media or described in fiction.
This all started with social learning - "children see, children do". I saw static approach in Fixers in PHP CS Fixer and I was making a Fixer. So why not use it? I felt in my guts it's not the best way to go, but I was not sure why and I didn't see anybody else using DI in CLI applications. Now I know why.
If you ever have a feeling that there is a better way to do things but you'll see that some Tomas Votruba is doing it differently, take your time - trust yourself, your intuition guides you for a reason. Question him and propose your idea, even though it might be crazy at the start. Maybe you'll save yourself and him a few PRs and many frustrated days from climbing up the legacy hole.
To prevent this 10 hours of trauma happening ever again, I made NoClassWithStaticMethodWithoutStaticNameRule that will look after our code.
❌
class SomeClass
{
public static function someFunction()
{
}
}
✅
class StaticSomeClass
{
public static function someFunction()
{
}
}
I've added this sniff to set before refactoring, scanned the code and added all found files to ignored. That way I knew what all classes need refactoring.
Static from Methods - One Step at a TimeI always do this in one single PR, starting with the simplest factory from ignored files above.
Remove the static in one factory:
class UseImportsTransformer
{
- public static function addNamesToTokens(...)
+ public function addNamesToTokens(...)
}
Pass it via constructor:
class RemoveUselessDocBlockFixer
{
+ /**
+ * @var UseImportsTransformer
+ */
+ private $userImportsTransformer;
+
+ public function __construct(UseImportsTransformer $userImportsTransformer)
+ {
+ $this->userImportsTransformer = $userImportsTransformer;
+ }
}
And use it in code:
-UseImportsTransformer::addNamesToTokens($this->newUseStatementNames, $tokens);
+$this->useImportsTransformer->addNamesToTokens($this->newUseStatementNames, $tokens);
I also admit that another code smell lead to this. In Symplify and Rector there is used Symfony 3.3 services architecture with autowiring and autodiscovery. State of art in PHP DI at the moment.
But Fixers and Checkers were exceptions. They were registered as services, but not autowired. So I was used to not-to add dependency to them manually, but via setters, new or ::static. It eventually and logically leads to this situation.
I learned something new and migrated to full-service approach in ECS 4.
::method(), but also new <class> and ::create().They also say that:
Wisdom is an ability to learn from others' mistakes.
So I hope you learned something new today!
Do you learn from my contents or use open-souce packages like Rector every day?
Consider supporting it on GitHub Sponsors.
I'd really appreciate it!
