4

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:pabc@ec2.eu-west-1.compute.amazonaws.com: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?

Fabien Snauwaert
  • 4,995
  • 5
  • 52
  • 70
  • I would not use any functions in defines. You can not guarantee the result will always be the same. In this case I would use different config files with hard coded values like a live.conf.php and a dev.conf.php. Depending on the environment you include the one or the other file. – Markus Zeller Mar 25 '20 at 18:02
  • But then I'd be committing the credentials to git, which is never a good idea. Using .env locally and Heroku in prod, don't want to stop using them. – Fabien Snauwaert Mar 25 '20 at 18:17
  • 2
    The concept is to have completely different configs. You could .gitignore them and just place an example.conf.php to the repository. – Markus Zeller Mar 25 '20 at 18:30
  • Are you using any dependency injection container (DI container)? / Optimal use of constants in a project: constants should be found either in _.env_ files, or as class constants. So no `define()`'s. / Do you really need to parse a db url? Suggestion: define the needed _.env_ constants (`DB_DSN`, `DB_USER`, `DB_PASSWORD`, etc) and use them directly, where they are needed. – PajuranCodes Mar 28 '20 at 13:27
  • @dakis Thanks for the tip about avoiding `define()`s. Not using any dependency injection yet. In prod, I'm on Heroku where the standard way to link an app to its database is through a `DATABASE_URL=postgres://u123:pabc@ec2.eu-west-1.compute.amazonaws.com:5432/dxyz` setting (super similar to .env), so I'd rather use a similar approach in dev. I guess I could move the parsing to a different file, and `putenv()`. – Fabien Snauwaert Mar 28 '20 at 13:48

2 Answers2

5

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.
PajuranCodes
  • 303
  • 3
  • 12
  • 43
Pieter van den Ham
  • 4,381
  • 3
  • 26
  • 41
  • I'm honestly lost about what Dependency Injection is, but at least this confirms this is something I need to clear up for myself. Thanks. – Fabien Snauwaert Apr 04 '20 at 14:18
  • 1
    @FabienSnauwaert You're probably overthinking it :) [Dependency injection is simply giving an object its instance variables in its constructor](https://www.jamesshore.com/Blog/Dependency-Injection-Demystified.html) – Pieter van den Ham Apr 04 '20 at 14:34
1

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.

Community
  • 1
  • 1
Kwadz
  • 2,206
  • 2
  • 24
  • 45
  • Thanks. I agree with point 1, but what's the argument for #2? The Twelve-Factor App was written by Adam Wiggins, Heroku's co-founder. The one-line database configuration I'm using is straight from Heroku: `DATABASE_URL=postgres://u123:pabc@ec2.eu-west-1.compute.amazonaws.com:5432/dxyz`; a point can be made for keeping it all in one line (makes editing/reusing/copying easier; when you need to change one of its values you typically need to change the others, too.) – Fabien Snauwaert Apr 04 '20 at 14:14
  • If you really want to use `define()`, you won't face your issue anymore. In your case the configuration expects the separated elements. It's simpler to add directly the expected parameters than something not expected that requires extra and unnecessary processing to parse the elements (which is a cause of your issue). In the case of a configuration that expects a `DATABASE_URL` param, like for Heroku or Symfony, then we just need this unique param. – Kwadz Apr 05 '20 at 09:35