Diving Into a Laravel Audit

We were to act as an independent third party, between a new studio who had inherited the codebase, and the client, who was anticipating launching the product.

The studio had some concerns about the quality of the code they'd received and wanted an independent review prior to picking up development on the project.

Here are a few thoughts on the process I followed and several takeaways I had from the experience:

TLDR; The most significant realization I had was how easy it is to let code degrade without a comprehensive test suite.

My process

As soon as I received the codebase, I began browsing through the folders, looking for the tests directory. While there were a few UI java-based tests, I’d estimate the coverage to represent less than 20% of the app. The developers were obviously clever individuals who had poured tremendous effort into their project, but perhaps due to time and budget constraints decided against following a Test Driven Development cycle. It didn’t take long to see the ramifications.

Next, I started browsing through the app folder, opening a few classes here and there. I tried to choose ones that seemed to represent the larger intent of the application, such as User.php, Team.php, or Business.php, so I could acquire an understanding of the main players.

Once I started to grasp the overarching concept of the application, I fired up PHPStorm and started running inspections, including PHP Mess Detector and PHP_CodeSniffer. If you aren’t familiar with either of these tools, I’d recommend checking them out as they provide a great starting point for diagnosing common PHP pitfalls.

PHP_CodeSniffer

Coding standards such as PSR-1 & PSR-2 are beneficial in maintaining unity between developers’ stylistic choices. Cleaner syntax is one step closer to reducing bugs, and PHP_CodeSniffer helps identify syntactical errors such as typos, improper indentation, and capitalization inconsistencies.

LINK: Getting PHP_CodeSniffer configured in PHPStorm

I want to be clear here… Using proper spelling for variable names, choosing specific and descriptive method names, and consistently formatting your code will not guarantee bug-free results. However, NOT coding “cleanly” will surely result in more problems with your code. PHP_CodeSniffer is a tool that can help locate these issues.

PHPMD

While PHP_CodeSniffer deals primarily with how the code looks, PHP Mess Detector analyzes the code logic by sniffing for problem areas. It helps identify possible bugs and over complicated expressions, and it locates unused parameters, methods, and properties.

After getting PHPMD installed and running in PHPStorm, I ran the inspection on the `App` directory.

To be fair, a large number of the errors were due to the developers not following a consistent syntactic standard; for example, the Mess Detector would error every time a variable wasn’t camel cased.

However, there were some enlightening errors thrown as well. Several of the classes were thousands of lines, contained more than 50 methods, and had methods that were hundreds of lines long. Again, this application had complex business logic, but the size of the classes and methods really surprised me.

Another handy feature of PHPMD is that it can evaluate the complexity of methods, both on the Cyclomatic Complexity, and the NPath Complexity (which is the number of possible paths through a given function). You can set the threshold and it will notify you of any methods that exceed it. There were many methods that not only exceeded the threshold, but doubled, tripled, or quadrupled it!

These classes would be very hard to test from a domain level. And who would ever want to make changes to a method that is that large? I’d be fearful that the smallest change could introduce rippling problems into the application.

Further digging

The PHPStorm inspections were very insightful as a starting point. After sifting through the results, I ended up with the following items that I felt needed to be addressed:

PSR Standardization - It would help the team to pick a PSR style and all use it consistently.
Namespaces - These were missing in all the first-party code.
Size of Classes - Too large and should be refactored into smaller units.
Size of Methods - Too complex and should be refactored into smaller units.
Polymorphism - There were more than 30 instances of switch statements, most of which could be replaced with polymorphism.
Repositories / Service classes - Business logic may be better organized using separate classes.
Static Class Usage - Static classes were used liberally through the app, making testing very difficult, if not impossible.
Dependency Injection - DI was not used, again hampering the ability to test the app.
Organization - Logic was interspersed throughout many different locations instead of being centralized into specific classes.
Security - Forms were susceptible to CSRF attacks.
Security - ACL was incomplete.

The deliverable

The PHP review was half of the audit, and another developer reviewed the Angular portion of the application. After compiling our findings, we created a 24 page report detailing the areas we felt needed to be addressed as well as several critical vulnerabilities.

What did I learn?

As soon as I’d opened a handful of the main classes, I gathered there was a lot going on in this application. I was also willing to admit that an application with complicated business logic would justifiably have more code than an application with simple business logic.

However, the combination of the automated PHPStorm inspections, and the overall size of the methods and classes underscored the fact that this project had grown into a tangled mess. Code was being duplicated, abstractions weren’t utilized, and classes weren’t much more than procedural logic containers.

What should be done about an application in this state?

Use tests. Use tests. Use tests.

Uncle Bob says, “keeping your code clean is not just cost-effective; it’s a matter of professional survival” and “test code is just as important as production code. It’s not a second-class citizen”. I couldn't agree more.

If the original development team had stuck with a TDD cycle, it’s likely the project wouldn’t have fallen into the state it was when we conducted the audit.

This application will be challenging to maintain over time since any changes to the PHP code will require manual QA to ensure the modifications haven’t broken another area of the application. The development cycles will be slow.

Postlude

As coders, we are an interesting bunch. We pour our heart and strength into our work, doing the very best we can with the tools we have and the knowledge we currently possess. That being said, it’s a somewhat easy task to sit back and harshly critique someone else's work, of whom you have no idea the surrounding pressures. Hopefully this audit was conducted in a manner that respects all involved and successfully moves the project forward!

You can find me on Twitter or in the Laravel Slack Channel to discuss this more!