remove usage of getenv from codebase (#1861)

UPGRADING.md

@@ -20,6 +20,7 @@ Perform EACH version specific task between your version and the new one, otherwi
 
 ### Developer
 
+- **BC break**: removed registration of `.env` with `putenv()` - do not rely on `getenv()` as it is not thread-safe
 - **BC break**: interface method signature `HtmlToPdfConverter::convertToPdf()` changed
 - **BC break**: the macros `badge` and `label` do not apply the `|trans` filter any more
 - **BC Break**: removed `getVisible()` (deprecated since 1.4) method on Customer, Project and Activity (use `isVisible()` instead, templates are still working)

bin/console

@@ -9,11 +9,10 @@ use Symfony\Component\Dotenv\Dotenv;
 
 set_time_limit(0);
 
-require __DIR__.'/../vendor/autoload.php';
+require dirname(__DIR__) . '/vendor/autoload.php';
 
-if (!isset($_SERVER['APP_ENV'])) {
+// do NOT rely on calls to getenv() as it is not thread safe
-    (new Dotenv(true))->load(__DIR__.'/../.env');
+(new Dotenv(false))->loadEnv(dirname(__DIR__).'/.env');
-}
 
 $input = new ArgvInput();
 $env = $input->getParameterOption(['--env', '-e'], $_SERVER['APP_ENV'] ?? 'prod');
@@ -22,9 +21,7 @@ $debug = (bool) ($_SERVER['APP_DEBUG'] ?? (in_array($env, ['dev', 'test']))) &&
 if ($debug) {
     umask(0000);
 
-    if (class_exists(Debug::class)) {
+    Debug::enable();
-        Debug::enable();
-    }
 }
 
 $kernel = new Kernel($env, $debug);

config/services.yaml

@@ -14,6 +14,7 @@ services:
                             # The best practice is to be explicit about your dependencies anyway.
         bind:
             $projectDirectory: '%kernel.project_dir%'
+            $kernelEnvironment: '%kernel.environment%'
 
     # makes classes in src/ available to be used as services
     # this creates a service per class whose id is the fully-qualified class name
@@ -42,6 +43,9 @@ services:
         arguments:
             $dashboard: '%kimai.dashboard%'
 
+    App\Configuration\MailConfiguration:
+        arguments: ['%env(MAILER_FROM)%']
+
     App\Configuration\LanguageFormattings:
         arguments: ['%kimai.languages%']
 

public/index.php

@@ -5,12 +5,10 @@ use Symfony\Component\ErrorHandler\Debug;
 use Symfony\Component\Dotenv\Dotenv;
 use Symfony\Component\HttpFoundation\Request;
 
-require __DIR__.'/../vendor/autoload.php';
+require dirname(__DIR__) . '/vendor/autoload.php';
 
-// The check is to ensure we don't use .env in production
+// do NOT rely on calls to getenv() as it is not thread safe
-if (!isset($_SERVER['APP_ENV'])) {
+(new Dotenv(false))->loadEnv(dirname(__DIR__) . '/.env');
-    (new Dotenv(true))->load(__DIR__.'/../.env');
-}
 
 $env = $_SERVER['APP_ENV'] ?? 'prod';
 $debug = (bool) ($_SERVER['APP_DEBUG'] ?? (in_array($env, ['dev', 'test'])));

src/Command/CreateReleaseCommand.php

@@ -28,14 +28,16 @@ class CreateReleaseCommand extends Command
     /**
      * @var string
      */
-    protected $rootDir = '';
+    private $rootDir = '';
-
     /**
-     * @param string $projectDirectory
+     * @var string
      */
-    public function __construct(string $projectDirectory)
+    private $environment;
+
+    public function __construct(string $projectDirectory, string $kernelEnvironment)
     {
         $this->rootDir = realpath($projectDirectory);
+        $this->environment = $kernelEnvironment;
         parent::__construct();
     }
 
@@ -51,14 +53,16 @@ class CreateReleaseCommand extends Command
             ->addOption('directory', null, InputOption::VALUE_OPTIONAL, 'Directory where the release package will be stored', '/tmp/')
             ->addOption('release', null, InputOption::VALUE_OPTIONAL, 'The version that should be zipped', Constants::VERSION)
         ;
+    }
 
-        /*
+    /**
-         * Hide this command in production.
+     * Make sure that this command CANNOT be executed in production.
-         * Maybe it should be de-activated completely?!
+     *
-         */
+     * @return bool
-        if (getenv('APP_ENV') === 'prod') {
+     */
-            $this->setHidden(true);
+    public function isEnabled()
-        }
+    {
+        return $this->environment !== 'prod';
     }
 
     /**
@@ -70,12 +74,6 @@ class CreateReleaseCommand extends Command
     {
         $io = new SymfonyStyle($input, $output);
 
-        if (getenv('APP_ENV') === 'prod') {
-            $io->error('kimai:create-release is not allowed in production');
-
-            return -2;
-        }
-
         $directory = $input->getOption('directory');
 
         if ($directory[0] === '/') {

src/Command/ResetCommand.php

@@ -29,6 +29,17 @@ use Symfony\Component\Console\Style\SymfonyStyle;
  */
 class ResetCommand extends Command
 {
+    /**
+     * @var string
+     */
+    private $environment;
+
+    public function __construct(string $kernelEnvironment)
+    {
+        $this->environment = $kernelEnvironment;
+        parent::__construct();
+    }
+
     /**
      * {@inheritdoc}
      */
@@ -55,7 +66,7 @@ EOT
      */
     public function isEnabled()
     {
-        return getenv('APP_ENV') !== 'prod';
+        return $this->environment !== 'prod';
     }
 
     /**

src/Configuration/MailConfiguration.php

@@ -11,14 +11,22 @@ namespace App\Configuration;
 
 class MailConfiguration
 {
+    /**
+     * @var string
+     */
+    private $mailFrom;
+
+    public function __construct(string $mailFrom)
+    {
+        $this->mailFrom = $mailFrom;
+    }
+
     public function getFromAddress(): ?string
     {
-        $from = getenv('MAILER_FROM');
+        if (empty($this->mailFrom)) {
-
-        if ($from === false || empty($from)) {
             return null;
         }
 
-        return $from;
+        return $this->mailFrom;
     }
 }

src/Controller/DoctorController.php

@@ -49,10 +49,15 @@ class DoctorController extends AbstractController
      * @var string
      */
     private $projectDirectory;
+    /**
+     * @var string
+     */
+    private $environment;
 
-    public function __construct(string $projectDirectory)
+    public function __construct(string $projectDirectory, string $kernelEnvironment)
     {
         $this->projectDirectory = $projectDirectory;
+        $this->environment = $kernelEnvironment;
     }
 
     /**
@@ -90,7 +95,7 @@ class DoctorController extends AbstractController
         return $this->render('doctor/index.html.twig', array_merge(
             [
                 'modules' => get_loaded_extensions(),
-                'dotenv' => $this->getEnvVars(),
+                'environment' => $this->environment,
                 'info' => $this->getPhpInfo(),
                 'settings' => $this->getIniSettings(),
                 'extensions' => $this->getLoadedExtensions(),
@@ -124,36 +129,26 @@ class DoctorController extends AbstractController
         return $results;
     }
 
-    private function getLogSize()
+    private function getLogSize(): int
     {
-        $logfileName = 'var/log/' . getenv('APP_ENV') . '.log';
+        $logfile = $this->getLogFilename();
-        $logfile = $this->projectDirectory . '/' . $logfileName;
 
-        return filesize($logfile);
+        return file_exists($logfile) ? filesize($logfile) : 0;
     }
 
     private function getLogFilename(): string
     {
-        // why is this check here ???
+        $logfileName = 'var/log/' . $this->environment . '.log';
-        if (!\in_array(getenv('APP_ENV'), ['test', 'dev', 'prod'])) {
-            throw new \RuntimeException('Unsupported log environment');
-        }
-
-        $logfileName = 'var/log/' . getenv('APP_ENV') . '.log';
 
         return $this->projectDirectory . '/' . $logfileName;
     }
 
-    private function getLog(int $lines = 100)
+    private function getLog(int $lines = 100): array
     {
-        try {
+        $logfile = $this->getLogFilename();
-            $logfile = $this->getLogFilename();
-        } catch (\Exception $ex) {
-            return ['ATTENTION: ' . $ex->getMessage()];
-        }
 
         if (!file_exists($logfile)) {
-            return ['ATTENTION: Missing logfile'];
+            return ['Missing logfile'];
         }
 
         if (!is_readable($logfile)) {
@@ -163,7 +158,7 @@ class DoctorController extends AbstractController
         $file = new \SplFileObject($logfile, 'r');
 
         if ($file->getSize() === 0) {
-            return ['Empty log'];
+            return ['Empty logfile'];
         }
 
         $file->seek($file->getSize());
@@ -173,8 +168,6 @@ class DoctorController extends AbstractController
         }
         $iterator = new \LimitIterator($file, $last_line - $lines, $last_line);
 
-        $result = [];
-
         try {
             $result = iterator_to_array($iterator);
         } catch (\Exception $ex) {
@@ -182,7 +175,7 @@ class DoctorController extends AbstractController
         }
 
         if (!is_writable($logfile)) {
-            $result[] = 'ATTENTION: Cannot write log file';
+            $result[] = 'ATTENTION: Logfile is not writable';
         }
 
         return $result;
@@ -210,14 +203,6 @@ class DoctorController extends AbstractController
         return $results;
     }
 
-    private function getEnvVars()
-    {
-        return [
-            'APP_ENV' => getenv('APP_ENV'),
-            'CORS_ALLOW_ORIGIN' => getenv('CORS_ALLOW_ORIGIN'),
-        ];
-    }
-
     private function getIniSettings()
     {
         $ini = [

src/DependencyInjection/Compiler/DoctrineCompilerPass.php

@@ -21,7 +21,7 @@ class DoctrineCompilerPass implements CompilerPassInterface
     /**
      * @var string[]
      */
-    protected $allowedEngines = [
+    private $allowedEngines = [
         'mysql',
         'sqlite'
     ];
@@ -33,20 +33,32 @@ class DoctrineCompilerPass implements CompilerPassInterface
     protected function findEngine()
     {
         $engine = null;
+        $databaseUrl = null;
 
-        if (null === $engine) {
+        if (null === $databaseUrl && isset($_ENV['DATABASE_URL'])) {
-            $dbConfig = explode('://', getenv('DATABASE_URL'));
+            $databaseUrl = $_ENV['DATABASE_URL'];
-            $engine = $dbConfig['0'] ?: null;
+        }
+
+        if (null === $databaseUrl && isset($_SERVER['DATABASE_URL'])) {
+            $databaseUrl = $_SERVER['DATABASE_URL'];
+        }
+
+        if (null === $databaseUrl && (false !== $envDbUrl = getenv('DATABASE_URL'))) {
+            $databaseUrl = $envDbUrl;
+        }
+
+        if (null !== $databaseUrl) {
+            $urlParts = explode('://', $databaseUrl);
+            $engine = $urlParts[0] ?: null;
         }
 
         if (null === $engine) {
             $engine = getenv('DATABASE_ENGINE');
         }
 
-        if (false === $engine) {
+        if (empty($engine)) {
             throw new \Exception(
-                'Could not detect database engine. Please set the environment config DATABASE_ENGINE ' .
+                'Could not detect database engine, make sure DATABASE_URL is available from $_SERVER or $_ENV. Check your .env file.'
-                'to one of: "' . implode(', ', $this->allowedEngines) . '" in your .env file, e.g. DATABASE_ENGINE=sqlite'
             );
         }
 

templates/doctor/index.html.twig

@@ -2,6 +2,7 @@
 {% import "doctor/actions.html.twig" as actions %}
 
 {% block page_title %}{{ 'menu.doctor'|trans }}{% endblock %}
+{% block page_subtitle %}Environment: <strong>{{ environment }}</strong>{% endblock %}
 {% block page_actions %}{{ actions.doctor('index') }}{% endblock %}
 
 {% block main %}
@@ -101,8 +102,9 @@
 
     {% embed '@AdminLTE/Widgets/box-widget.html.twig' with {collapsed: true} %}
         {% block box_title %}Composer packages{% endblock %}
+        {% block box_body_class %}no-padding{% endblock %}
         {% block box_body %}
-            <table class="table">
+            <table class="table table-hover">
                 {% for name, value in composer %}
                     <tr>
                         <th style="width:15%">{{ name }}</th>
@@ -113,24 +115,11 @@
         {% endblock %}
     {% endembed %}
     
-    {% embed '@AdminLTE/Widgets/box-widget.html.twig' with {collapsed: true} %}
-        {% block box_title %}Environment variables{% endblock %}
-        {% block box_body %}
-            <table class="table">
-                {% for name, value in dotenv %}
-                    <tr>
-                        <th style="width:15%">{{ name }}</th>
-                        <td>{{ value }}</td>
-                    </tr>
-                {% endfor %}
-            </table>
-        {% endblock %}
-    {% endembed %}
-
     {% embed '@AdminLTE/Widgets/box-widget.html.twig' with {collapsed: true} %}
         {% block box_title %}PHP{% endblock %}
+        {% block box_body_class %}no-padding{% endblock %}
         {% block box_body %}
-            <table class="table">
+            <table class="table table-hover">
                 <tr>
                     <th style="width:15%">Version</th>
                     <td>{{ constant('PHP_VERSION') }}</td>
@@ -157,8 +146,9 @@
 
     {% embed '@AdminLTE/Widgets/box-widget.html.twig' with {collapsed: true} %}
         {% block box_title %}Server{% endblock %}
+        {% block box_body_class %}no-padding{% endblock %}
         {% block box_body %}
-            <table class="table">
+            <table class="table table-hover">
                 {% for name, value in info %}
                     <tr>
                         <th style="width:15%">{{ name }}</th>

tests/Command/CreateReleaseCommandTest.php

@@ -19,21 +19,20 @@ use Symfony\Bundle\FrameworkBundle\Test\KernelTestCase;
  */
 class CreateReleaseCommandTest extends KernelTestCase
 {
-    /**
-     * @var Application
-     */
-    protected $application;
-
-    protected function setUp(): void
-    {
-        $kernel = self::bootKernel();
-        $this->application = new Application($kernel);
-        $this->application->add(new CreateReleaseCommand(realpath(__DIR__ . '/../../')));
-    }
-
     public function testCommandName()
     {
-        $command = $this->application->find('kimai:create-release');
+        $kernel = self::bootKernel(['environment' => 'test']);
+        $application = new Application($kernel);
+        $application->add(new CreateReleaseCommand(realpath(__DIR__ . '/../../'), 'test'));
+
+        $command = $application->find('kimai:create-release');
+        self::assertTrue($command->isEnabled());
         self::assertInstanceOf(CreateReleaseCommand::class, $command);
     }
+
+    public function testCommandNameIsNotAvailableInProd()
+    {
+        $command = new CreateReleaseCommand(realpath(__DIR__ . '/../../'), 'prod');
+        self::assertFalse($command->isEnabled());
+    }
 }

tests/Command/ResetCommandTest.php

@@ -19,21 +19,19 @@ use Symfony\Bundle\FrameworkBundle\Test\KernelTestCase;
  */
 class ResetCommandTest extends KernelTestCase
 {
-    /**
-     * @var Application
-     */
-    protected $application;
-
-    protected function setUp(): void
-    {
-        $kernel = self::bootKernel();
-        $this->application = new Application($kernel);
-        $this->application->add(new ResetCommand());
-    }
-
     public function testCommandName()
     {
-        $command = $this->application->find('kimai:reset-dev');
+        $kernel = self::bootKernel();
+        $application = new Application($kernel);
+        $application->add(new ResetCommand('test'));
+
+        $command = $application->find('kimai:reset-dev');
         self::assertInstanceOf(ResetCommand::class, $command);
     }
+
+    public function testCommandNameIsNotEnabledInProd()
+    {
+        $command = new ResetCommand('prod');
+        self::assertFalse($command->isEnabled());
+    }
 }

tests/Configuration/MailConfigurationTest.php

@@ -19,15 +19,13 @@ class MailConfigurationTest extends TestCase
 {
     public function testGetFromAddress()
     {
-        $previous = getenv('MAILER_FROM');
+        $sut = new MailConfiguration('foo-bar123@example.com');
-        putenv('MAILER_FROM=foo-bar123@example.com');
-
-        $sut = new MailConfiguration();
         self::assertEquals('foo-bar123@example.com', $sut->getFromAddress());
+    }
 
-        putenv('MAILER_FROM=');
+    public function testGetFromAddressWithEmptyAddressReturnsNull()
+    {
+        $sut = new MailConfiguration('');
         self::assertNull($sut->getFromAddress());
-
-        putenv('MAILER_FROM=' . $previous);
     }
 }

tests/Controller/DoctorControllerTest.php

@@ -32,6 +32,6 @@ class DoctorControllerTest extends ControllerBaseTest
         $this->assertAccessIsGranted($client, '/doctor');
 
         $result = $client->getCrawler()->filter('.content .box-header');
-        $this->assertEquals(7, \count($result));
+        self::assertCount(6, $result);
     }
 }