PSR-1: 2.3. Side Effects: variable inside config file

664 views Asked by At

PSR-1 includes recommendation 2.3. Side Effects:

A file SHOULD declare new symbols (classes, functions, constants, etc.) and cause no other side effects, or it SHOULD execute logic with side effects, but SHOULD NOT do both.

Consider this example (my own) inside of a config.php file:

/**
 * Parsing the database URL.
 * DATABASE_URL is in the form:
 *  postgres://user:password@hostname:port/database
 * e.g.:
 *  postgres://u123:[email protected]:5432/dxyz
 */
$url = parse_url(getenv('DATABASE_URL'));
define('DB_HOST', $url['host']);
define('DB_NAME', substr($url['path'], 1)); // get rid of initial slash
define('DB_USER', $url['user']);
define('DB_PASSWORD', $url['pass']);

If I do this, I'm effectively not respecting the recommendation. phpcs will, rightfully, complain about it, because of the variable:

FILE: config.php
-----------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
-----------------------------------------------------------------------------------------------------------------
 1 | WARNING | A file should declare new symbols (classes, functions, constants, etc.) and cause no other side
   |         | effects, or it should execute logic with side effects, but should not do both. The first symbol
   |         | is defined on line 17 and the first side effect is on line 162.
-----------------------------------------------------------------------------------------------------------------

An alternative would be this:

define('DB_HOST', parse_url(getenv('DATABASE_URL'))['host']);
define('DB_NAME', substr(parse_url(getenv('DATABASE_URL'))['path'], 1));
define('DB_USER', parse_url(getenv('DATABASE_URL'))['user']);
define('DB_PASSWORD', parse_url(getenv('DATABASE_URL'))['pass']);

No variable, no problem. But this is WET and hard to read.

I understand the recommendation is just that, and that it says "SHOULD", not "MUST". But this still bugs me… For one thing, anytime I check the file phpcs will complain about it, but report it just once per line, leaving the door open to adding more "side effects" which have no place in a config file.

I'm still new to this whole PSR thing.

Did I miss any clever way to get rid of the variable, while keeping things readable?

A corollary would be: how do serious projects, that insist on following recommendations to the letter, handle this?

2

There are 2 answers

2
Pieter van den Ham On BEST ANSWER

1. It's fine, don't sweat it

You already mention it in your question, but this recommendation is a SHOULD and not a MUST. If this is the only PSR-1 issue in your entire project: good job!

But your question was: how do other projects go about this?

2. Move away from defines for configuration

Global constants, when used incorrectly, are dependency magnets. They introduce coupling and make your code harder to digest. This Q&A is a very good read on why you should move away from them.

Use dependency injection instead (yes, scalar configuration constants are also dependencies).

3. Case study: Symfony

Symfony-based projects use:

  • either YAML (recommended) or XML configuration files to configure the dependency injection container, along with
  • environment variables, to set the configuration options specific to each environment in which the application should run. These env vars are defined in environment-specific .env files.

For example, to configure a Database service in a Symfony project you'd create a YAML file that contains:

services:
    My\Database\Factory: # <-- the class we are configuring
        arguments:
            $url: '%env(DATABASE_URL)' # <-- configure the $url constructor argument

Symfony compiles this into PHP code, injecting the DATABASE_URL environment variable into the class that requires it.

You would then parse DATABASE_URL in the constructor of the My\Database\Factory class, and use the result to construct your database class.

Pros:

  • Configuration is separated from code
  • Configuration is easy to change
  • Configuration is easy to read

Cons:

  • Dependency injection and using a DI container has a learning curve and requires a change in the way you think about constructing objects.
2
Kwadz On

As stated in the twelve-factors app methodology:

Apps sometimes store config as constants in the code. This is a violation of twelve-factor, which requires strict separation of config from code. Config varies substantially across deploys, code does not.

The twelve-factor app stores config in environment variables. Env vars are easy to change between deploys without changing any code

You're on the right track about best practices, you just need to correct some mistakes.

1. Use variables for environment variables

You want to use constants for things that are not. The value of the database name can vary according to the environment. It's NOT a constant, it's a (environment) variable, you should use $dbName = getenv('DB_NAME').
In contrast, the number π is a constant, it will never change and can be hardcoded.

You can have a look at the source code of open-source projects like Composer or the Symfony components, you'll see getenv() used to populate variables only.

2. Use directly the elements expected in the configuration

In your case you shouldn't use the full database URL as a single configuration item. You should instead separate each element in environment variables like DB_HOST, DB_NAME, DB_PORT, as expected by the configuration.