Skip to content

Expand .env values conversion ability of BaseConfig - #5704

Closed
paulbalandan wants to merge 3 commits into
codeigniter4:developfrom
paulbalandan:base-config-conversion
Closed

Expand .env values conversion ability of BaseConfig#5704
paulbalandan wants to merge 3 commits into
codeigniter4:developfrom
paulbalandan:base-config-conversion

Conversation

@paulbalandan

Copy link
Copy Markdown
Member

Description
Initial (current):

'true' => true
'false' => false
all other strings => trimmed

Additional (proposed):

'12' => 12
'1.25' => 1.25
'null' => null
'empty' => ''

Supersedes and closes #5691
Fixes #5688

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@paulbalandan
paulbalandan force-pushed the base-config-conversion branch from edd76a4 to 2ada1e4 Compare February 17, 2022 12:00
@iRedds

iRedds commented Feb 17, 2022

Copy link
Copy Markdown
Collaborator

I vote for php.
Type of variables out of the box.
No extra parsing needed.
=)

Comment on lines +38 to +39
SimpleConfig.shortie = '12.45'
SimpleConfig.longie = '250'

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think '12.45' or "12.45" is string, 12.45 is float.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

So you mean drop the trimming of quotes? And regard encapsed numbers as intended to be strings?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

As a PHP user, I think '12.45' is a string, not an integer.
It is quoted intentionally.

I think adding the following two cases are enough.
SimpleConfig.shortie = 12.45 as float, and SimpleConfig.longie = 250 as integer.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

However, inside a .env file where everything is string, quotes really serve no purpose other than capturing in-between spaces. That's why in DotEnv::sanitizeValue() we capture what's inside those quotes.

$value = preg_replace($regexPattern, '$1', $value);

So, in the absence of a whitespace in between, 12.45 === '12.45'
And in the default env, you can write strings with or without quotes. It doesn't matter as long there is no whitespace.

database.tests.password = root

contentsecuritypolicy.styleSrc = 'self'

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this is an opportunity to standardize, and I like the ability to retain numerical strings. I can't think off the top of my head where that might be important (maybe with strongly typed Config properties?) but my hunch is we need to support it and checking for the presence of quotes is a decent approach.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The current code does not matter.
12.45 === '12.45' is wired.
And there is no way to set numerical strings.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

First of all, all environment variable's values are strings in PHP.
And we don't have to use DotEnv, we can set environment variables to web server.
So the DotEnv code does not matter.

Now we are going to change the interpretation of the values. We already changed when true/false.
We can decide the behavior for us.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

DotEnv matters because in the current application life cycle, DotEnv sets the env variables and BaseConfig fetches them. Well, any experienced developer can set the env variables themselves but I don't think majority of CI4 users will do that. They just rely on the default implementation.

Okay, since both of you want now to distinguish between integer 12 and string 12 by way of encapsulating quotes, I need to change the behavior of DotEnv (which I'll do in a separate PR) and adapt the code here accordingly.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, integer 12 and string 12 are different.
Not being able to use numeric strings for properties in the Config files is too big a restriction.

I just wanted to say, the following two are different things. They are related and must work with, but not one.

  • DotEnv reads .env and sets environment variables in PHP
  • BaseConfig fetches environment variables and sets properties

@kenjis

kenjis commented Feb 17, 2022

Copy link
Copy Markdown
Member

@iRedds See #5688 (comment)
The current implementation sets string values to integer properties.

@iRedds

iRedds commented Feb 17, 2022

Copy link
Copy Markdown
Collaborator

The type cast must be inside.
What if I want "0" to be a string, not converted to an integer.

@kenjis

kenjis commented Feb 17, 2022

Copy link
Copy Markdown
Member

The type cast must be inside.

What do you mean?

At least, I disagree with the current Paul's implementation, which both SimpleConfig.shortie = '12.45' and SimpleConfig.shortie = 12.45 are converted to float.

@iRedds

iRedds commented Feb 17, 2022

Copy link
Copy Markdown
Collaborator

The type cast must be inside.

What do you mean?

$this->propertyExpectsInt = (int) env('property');

@MGatner MGatner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

One other thing you may have accounted for but just to be sure, from https://www.php.net/manual/en/function.is-numeric :

8.0.0 Numeric strings ending with whitespace ("42 ") will now return true. Previously, false was return instead

Comment thread system/Config/BaseConfig.php
@paulbalandan

Copy link
Copy Markdown
Member Author

One other thing you may have accounted for but just to be sure, from https://www.php.net/manual/en/function.is-numeric :

8.0.0 Numeric strings ending with whitespace ("42 ") will now return true. Previously, false was return instead

The current and proposed behavior do not remove edge whitespaces, only edge quotes.
https://3v4l.org/p1At6

So, your example will have different values on php 8.

@kenjis

kenjis commented Feb 17, 2022

Copy link
Copy Markdown
Member

$this->propertyExpectsInt = (int) env('property');

The BaseConfig provides the feature to set properties of the object.

$this->initEnvValue($this->{$property}, $property, $prefix, $shortPrefix);

protected function initEnvValue(&$property, string $name, string $prefix, string $shortPrefix)

@@ -1,4 +1,3 @@
SimpleConfig.QZERO=0
SimpleConfig.QZEROSTR="0"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why deleted?

@kenjis

kenjis commented Feb 17, 2022

Copy link
Copy Markdown
Member

Please add test cases like these:

Foo.bar = '02471'
Foo.bar = 02471
Foo.bar = '1337e0'
Foo.bar = 1337e0
Foo.bar = ' 42'
Foo.bar = '42 '

@samsonasik

Copy link
Copy Markdown
Member

@paulbalandan for database use case, is this will still works for password db: 123456 without quote?

database.default.password = 123456

@kenjis kenjis added breaking change Pull requests that may break existing functionalities bug Verified issues on the current code behavior or pull requests that will fix them labels Feb 20, 2022
@paulbalandan
paulbalandan marked this pull request as draft February 22, 2022 03:45
@kenjis

kenjis commented Apr 12, 2022

Copy link
Copy Markdown
Member

#5779 was merged.

@kenjis kenjis closed this Apr 12, 2022
@paulbalandan
paulbalandan deleted the base-config-conversion branch April 17, 2022 12:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking change Pull requests that may break existing functionalities bug Verified issues on the current code behavior or pull requests that will fix them

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: Ajax request makes the session expire when app.sessionExpiration is set to 0

5 participants