Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: [BaseConfig] string value is set from environment variable even if it should be int/float #5779

Merged
merged 3 commits into from
Apr 12, 2022
Merged

fix: [BaseConfig] string value is set from environment variable even if it should be int/float #5779

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5b528af
fix: now initEnvValue() converts type when the default property value…
kenjis Mar 6, 2022
8524bb9
test: add test for nullable int property
kenjis Mar 18, 2022
d61b3c7
test: change .env entry
kenjis Mar 19, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 17 additions & 5 deletions system/Config/BaseConfig.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ public function __construct()
*
* @param mixed $property
*
* @return mixed
* @return void
*/
protected function initEnvValue(&$property, string $name, string $prefix, string $shortPrefix)
{
Expand All @@ -102,16 +102,28 @@ protected function initEnvValue(&$property, string $name, string $prefix, string
} elseif ($value === 'true') {
$value = true;
}
$property = is_bool($value) ? $value : trim($value, '\'"');
}
if (is_bool($value)) {
$property = $value;

return;
}

$value = trim($value, '\'"');

return $property;
if (is_int($property)) {
$value = (int) $value;
} elseif (is_float($property)) {
$value = (float) $value;
}

$property = $value;
}
}

/**
* Retrieve an environment-specific configuration setting
*
* @return mixed
* @return string|null
*/
protected function getEnvValue(string $property, string $prefix, string $shortPrefix)
{
Expand Down
22 changes: 22 additions & 0 deletions tests/system/Config/BaseConfigTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,25 @@ public function testBasicValues()
$this->assertSame(18, $config->golf);
}

public function testUseDefaultValueTypeIntAndFloatValues()
{
$dotenv = new DotEnv($this->fixturesFolder, '.env');
$dotenv->load();
$config = new SimpleConfig();

$this->assertSame(0.0, $config->float);
$this->assertSame(999, $config->int);
}

public function testUseDefaultValueTypeStringValue()
{
$dotenv = new DotEnv($this->fixturesFolder, '.env');
$dotenv->load();
$config = new SimpleConfig();

$this->assertSame('123456', $config->password);
}

/**
* @runInSeparateProcess
* @preserveGlobalState disabled
Expand Down Expand Up @@ -98,6 +117,9 @@ public function testEnvironmentOverrides()
$this->assertSame('bar', $config->onedeep_value);
// array property name with underscore and key with underscore
$this->assertSame('foo', $config->one_deep['under_deep']);

// The default property value is null but has type
$this->assertSame(20, $config->size);
}

public function testPrefixedValues()
Expand Down
7 changes: 7 additions & 0 deletions tests/system/Config/fixtures/.env
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,3 +32,10 @@ SimpleConfig.crew.captain = Malcolm
SimpleConfig.crew.pilot = Wash
SimpleConfig.crew.comms = true
SimpleConfig.crew.doctor = false

SimpleConfig.float = '0.0'
SimpleConfig.int = '999'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per your #5704 (comment), this should result in strings since encapsed in quotes. However, in lines 64-65 of the tests, those are tested as float and int respectively.

Isn't it contradictory? The reason why my PR was stalled is because of trying to find a solution to that distinction (i.e., string 12 and int 12) and this PR is not addressing that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I've changed my mind.

After all, the correct type is the property type in the Config class.

But I don't recommend to use ' for int/float value, I change the test fixture.


SimpleConfig.password = 123456

SimpleConfig.size=20
4 changes: 4 additions & 0 deletions tests/system/Config/fixtures/SimpleConfig.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,4 +48,8 @@ class SimpleConfig extends \CodeIgniter\Config\BaseConfig
public $one_deep = [
'under_deep' => null,
];
public $float = 12.34;
public $int = 1234;
public $password = 'secret';
public ?int $size = null;
}