Does the AppEnvironment case value 'prod' bother anyone? #1150
Replies: 4 comments
-
Good point! The inconsistency between the case name and the actual value can be confusing. Ideally, the enum could support more flexible mappings, so that both One way to improve this could be by allowing multiple aliases for each environment. For example, If you have some time, it would be awesome if you could help contribute to improving this injector by adding alias support or proposing other improvements that make it more intuitive for developers. Thanks for raising this! Here’s an example of how the enum could be improved to handle multiple aliases for environment names: <?php
declare(strict_types=1);
namespace Spiral\Boot\Environment;
use Spiral\Boot\EnvironmentInterface;
use Spiral\Boot\Injector\ProvideFrom;
use Spiral\Boot\Injector\InjectableEnumInterface;
#[ProvideFrom(method: 'detect')]
enum AppEnvironment: string implements InjectableEnumInterface
{
case Production = 'prod';
case Stage = 'stage';
case Testing = 'testing';
case Local = 'local';
private const ALIASES = [
'prod' => self::Production,
'production' => self::Production,
'prd' => self::Production,
'stage' => self::Stage,
'stg' => self::Stage,
'testing' => self::Testing,
'test' => self::Testing,
'local' => self::Local,
'dev' => self::Local,
];
public function isProduction(): bool
{
return $this === self::Production;
}
public function isTesting(): bool
{
return $this === self::Testing;
}
public function isLocal(): bool
{
return $this === self::Local;
}
public function isStage(): bool
{
return $this === self::Stage;
}
public static function detect(EnvironmentInterface $environment): self
{
$value = $environment->get('APP_ENV');
if (\is_string($value) && isset(self::ALIASES[$value])) {
return self::ALIASES[$value];
}
return self::Local;
}
} |
Beta Was this translation helpful? Give feedback.
-
I think in an ideal scenario where there are no backward compatibility issues, this should really be a don't ask, tell solution. An enum is a finite set of constants. If the supplied value does not exist, it should throw a LogicException. Currently, if a user supplies a custom APP_ENV value, it defaults to Local, which is unexpected. Since AppEnvironment is an enum, supplied values should explicitly use one of the enum values. I don't see the need for a user to choose between prd, prod, and production; they are all the same thing. It would greatly simplify everything if only a valid enum value could be used. I would almost suggest to change I'm not sure of the impact of that suggestion is, because to be honest im not deeply familiar with your code base yet, i am just in the midst of cheking it out because i might switch from my projects from Laravel to Spiral. |
Beta Was this translation helpful? Give feedback.
-
When I first started this caught me off guard twice, first time in production and second in testing The cleanest way I think to remedy this, update |
Beta Was this translation helpful? Give feedback.
-
See #1170 |
Beta Was this translation helpful? Give feedback.
-
Is 'prod' a more common convention in a code base over 'production'?
case Production = 'prod';
If 'prod' is more of a common convention why is the case name
Production
?And if in your env you set APP_ENV=production the AppEnvironment detect method will return 'local'.
The case name is
Production
, the method isisProduction()
but the value is short handprod
..I know it's just personal opinion but it just bothers me, lol.
Beta Was this translation helpful? Give feedback.
All reactions