Getting rid of cronner.module

Background: This blog post is part 2 of musings around legacy code on violinist.io. Violinist.io is an automated dependency updater for PHP/Composer. It's a SaaS built on Drupal 8, now running on Drupal 10, in its eigth year. In this second post I am looking at one way to approach legacy code, and how to use static analysis and test driven development to safely refactor and remove legacy code.

Some months ago I wrote about the combined feeling of shame and respect that surrounds legacy code. In an attempt to get rid of the legacy code in what is called cronner.module, I will start from the top of the .module file, and work my way towards removing it completely. From time to time, that can also result in a good blog post, I thought. Here's a blog post about that.

The first lines of the module file is this:

<?php

/**
 * @file
 * Cronner module.
 *
 * Bad name, but what are you going to do, right?
 */

The code here sounds a bit resigned when it asks about what are you going to do. But in this blog post, what are we going to do? We will start the process of getting rid of all of it, that's what we will do!

The first few lines of actual code are constant definitions. Here's the first one:

define('CRONNER_STATE_KEY_PREFIX', 'cronner_state.node.');

Looking at the git history I can see this constant was added in a commit around the time of the public beta with a very little helpful commit message:

From: 09c138732a43a6627b621fff74fbef5ea0ad0d1c
Date: Fri, 28 Apr 2017 17:07:29 +0200
Subject: [PATCH] Hm, this thing

The commit message does not help us with what the constant is used for. So far we can speculate, but it seems to be a prefix. And it's used for storing some state about nodes. Defining constants like this might seem strange if you are a bit new to Drupal, but this way of defining constants was canonical in Drupal 7 and below, but also followed a lot of codebases into the Drupal 8 era. Drupal core as well actually. One such example is the constant FILE_EXISTS_RENAME which lived on until Drupal 9 (deprecated in 8.7.0). Anyway I digress.

What we want then, is to remove this one constant. Let's just try that and see what breaks? Here's the output from phpunit:

There were 19 errors:

…

Error: Undefined constant "CRONNER_STATE_KEY_PREFIX”

The unit tests are failing. 19 of them. Good start, we have decent unit test coverage. Let's see if our integration tests fail?

xxx scenarios (xx passed, 112 failed)
xxxx steps (xxxx passed, 49 failed, xxx skipped)
xxmxx.xxs (90.59Mb)

112 scenarios failed in our behat test suite. Great indication that we can remove things safely! Lastly let's do some static analysis with PHPStan:

  Line   web/modules/custom/cronner/cronner.module                            
 ------ --------------------------------------------------------------------- 
  xxx    Constant CRONNER_STATE_KEY_PREFIX not found.

As expected, PHPStan also informs me about this change being a problem. Thanks PHPStan! It also indicates that among the scanned files, the constant is only used once. It's used like this:

/**
 * Gets the state key of a node.
 */
function _cronner_get_state_key(NodeInterface $node) {
  return CRONNER_STATE_KEY_PREFIX . $node->id();
}

In addition to the comment being very little helpful, this tells me I have just encountered another part of the cronner module to remove. Let's remove this entire function as well since we already know the function must be covered by both unit tests and integration tests. Then let’s go ahead and use a combination of TDD and static analysis (with PHPStan) to make sure our refactoring is successful. I remove the function and re-run PHPStan:

Function _cronner_get_state_key not found.          

…

 [ERROR] Found 7 errors

PHPStan is helpfully pointing out all of the 7 places I should start with the refactoring. I look through some of them and see the usages are quite connected to some of the other constants in the beginning of the file:

define('CRONNER_PROJECT_NEW', 'new');
define('CRONNER_PROJECT_QUEUED', 'queued');
define('CRONNER_PROJECT_RUNNING', 'running');
define('CRONNER_PROJECT_PROCESSED', 'processed');
define('CRONNER_PROJECT_ERRORED', 'errored');
define('CRONNER_PROJECT_UNKNOWN', 'unknown');

My mother always used to say. When you are tidying up your room it’s best to keep going while you are somewhat effective in deciding what to get rid of. If you find some of your old toys and start to play with them, it’s an indication that the tidying session is heading towards an unproductive state. And right now I feel effective and decide right away I am removing those constants as well. I guess that also makes the analogy to me tidying my room as a kid kind of weird, since the opposite would mean these constants were my toys, and I end up playing with them? I mean, I do end up playing with old code from time to time, but constants? No fun playing with.

But looking at these old toys, it also becomes obvious what we use these constants for. Node state could mean all kinds of things, right? This is used to store and update the job status of the projects. Like if they are currently queued, if they are currently running and so on.

So it turns out that while removing this, this actually seems like a good opportunity to clean up some of that code and make it a bit more modern. What I usually do when cleaning up custom code like this is to put as much as possible on drupal.org as open source code. This is the case now as well. So I open an issue to create a project run status service, and start to refactor the custom code and logic surrounding the removals of constants and functions in cronner.module. It can be found here: https://www.drupal.org/project/violinist_projects/issues/3453459.

Now I go ahead and start using this service. It quickly becomes obvious that some of the constants are also used in a map to display a human readable status to the user:

/**
 * Gets a human readable version of a status constant.
 *
 * @param string $status
 *   A status constant.
 *
 * @return string
 *   Something more sensible.
 */
function cronner_get_human_status($status) {
  $map = [
    CRONNER_PROJECT_UNKNOWN => t('Unknown'),
    CRONNER_PROJECT_ERRORED => t('Errored'),
    CRONNER_PROJECT_PROCESSED => t('Processed'),
    CRONNER_PROJECT_RUNNING => t('Running'),
    CRONNER_PROJECT_NEW => t('New'),
    CRONNER_PROJECT_QUEUED => t('Queued'),
  ];
  return !empty($map[$status]) ? $map[$status] : $status;
}

There’s also methods for getting a state from a node, and setting it. Which I am also removing, and finding usages of in static analysis:


  xxx    Function cronner_set_state not found.                                                 
         💡 Learn more at https://phpstan.org/user-guide/discovering-symbols                   
  xxx    Function _cronner_get_state_key not found.                                            
         💡 Learn more at https://phpstan.org/user-guide/discovering-symbols                 
  xxx    Function cronner_get_human_status not found.                                          
         💡 Learn more at https://phpstan.org/user-guide/discovering-symbols

Or in unit tests like so:

ReplaceTokensTest::testClaimJobAndTokenReplaced

Error: Call to undefined function cronner_set_state()

Instead of these calls to getting the state, setting the state, and getting the human readable state, I am now using the newly created service. When I started writing this blog post I was very much looking forward to summarizing the numbers of deleted lines and how great that was, but in practice some of the changes are actually adding lines to the module:

-  cronner_set_state($node, CRONNER_PROJECT_PROCESSED);
+  /** @var \Drupal\violinist_projects\ProjectRunStatus $run_status_service */
+  $run_status_service = \Drupal::service('violinist_projects.run_status');
+  $run_status_service->setRunStatusForProject($node, ProjectRunStatusValue::STATUS_PROCESSED);

Jokes aside. After some successful deletions and refactorings into using a service I am looking at 99 deletions and 55 additions. A net positive result I would say. Unfortunately there is still quite a way to go before I can go ahead and delete the entire folder called “cronner”. But at least I managed to refactor the codebase and delete the first 7 lines of constants in the file.

To celebrate this tiny step towards getting rid of cronner.module I tried to search for cronner on giphy. No hits, at this point. Oh well, here is an animated gif that supposedly illustrates “cronn”