dproofreaders icon indicating copy to clipboard operation
dproofreaders copied to clipboard

Introduce a new SiteConfig class

Open cpeel opened this issue 8 months ago • 0 comments

Rather than use untyped globals everywhere, pull configuration values from a SiteConfig class. The class still sources from site_vars.php so it doesn't break the way the site is configured with the existing deployment scripts.

This new site configuration infrastructure (see https://github.com/DistributedProofreaders/dproofreaders/issues/1256) will allow us to:

  1. stop using global variables -- beyond being a best practice, this makes phpunit much happier
  2. have the configuration variables be typed (yay!)
  3. allow variables to have default values, allowing us to add new ones more easily

Now, instead of the config values being in the global context, they are only accessible in the SiteConfig static object, so:

global $preceding_proofer_restriction;
echo $preceding_proofer_restriction;

becomes

echo SiteConfig::get()->preceding_proofer_restriction;

The new method re-uses the existing site_vars.php file which means there are no changes required to how the code is configured, just how we use those configuration variables. If we went forward with this approach, a second PR would later move away from the bash-based configuration model to just using the PHP file directly.

Keeping the configurations in a PHP file was intentional because PHP files are cached in the opcache and do not need to go to disk or be processed upon access, unlike a yaml, json, or ini-based file.

Other approaches considered:

  • I evaluated using an existing package like Symfony's Config package but it's huge and very complex and I don't think we need that complexity.
  • The initial version of this used static variables (https://github.com/DistributedProofreaders/dproofreaders/pull/1450) but I think using a singleton method allows more flexibility later.
  • A version with readonly variables was considered (https://github.com/DistributedProofreaders/dproofreaders/pull/1465) but needing to separate the default values from the variable declaration seems kludgy. This singleton version allows us to reconsider that later if we so choose.

Sandbox in https://www.pgdp.org/~cpeel/c.branch/site-config-singleton/

cpeel avatar Apr 06 '25 15:04 cpeel