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.

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

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

Conversation

kenjis
Copy link
Member

@kenjis kenjis commented Mar 6, 2022

Description
Fixes #5688
Supersede #5704, #5691 and #5690

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

@kenjis kenjis changed the title fix: string value is set from environment variable even if it should be int/float fix: [BaseConfig] string value is set from environment variable even if it should be int/float Mar 6, 2022
@kenjis kenjis added the bug Verified issues on the current code behavior or pull requests that will fix them label Mar 6, 2022
Copy link
Member

@MGatner MGatner left a comment

Choose a reason for hiding this comment

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

This is much cleaner, but what about null handling? I think it could be quite common to have a Config with public property $foo; as int|null and since no initial value was set we would end up with a string.

@kenjis
Copy link
Member Author

kenjis commented Mar 14, 2022

@MGatner

what about null handling?

This does nothing.

I think it could be quite common to have a Config with public property $foo; as int|null

Not so common. I think it might be bad design.
Now null is converted to a string. This PR changes nothing about null handling.
It is out of scope.

Do you really need null handling?

@MGatner
Copy link
Member

MGatner commented Mar 17, 2022

Sorry, I realize my description and example were both ambiguous. I meant what if the current type is null. An example:

app/Config/Widget.php

namespace Config;

class Widget extends BaseConfig
{
    property ?int $size = null;
}
.env

Widget.size=20

@kenjis kenjis force-pushed the fix-baseconfig-type-conversion branch from fe778b9 to 82a5107 Compare March 18, 2022 07:09
@kenjis kenjis force-pushed the fix-baseconfig-type-conversion branch from 82a5107 to 8524bb9 Compare March 18, 2022 07:11
@kenjis
Copy link
Member Author

kenjis commented Mar 18, 2022

@MGatner If you type the property like your sample, it is no problem.
See the test passes.

If you don't type it like the following, string 20 is set in the $size.

class Widget extends BaseConfig
{
    property $size = null;
}

@kenjis
Copy link
Member Author

kenjis commented Mar 18, 2022

It seems typed properties would solve most of the type problems.
So I think we don't need to add extra logic to convert types.

This PR is enough to the current problems.

Comment on lines 36 to 37
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.

Do not recommend to use `'` for int/float value.
@kenjis kenjis requested a review from paulbalandan April 1, 2022 01:05
@kenjis
Copy link
Member Author

kenjis commented Apr 1, 2022

@paulbalandan Can I merge this?
Or you still have something wrong?

@MGatner
Copy link
Member

MGatner commented Apr 10, 2022

@paulbalandan still keen on your final opinion. @kenjis if he is unavailable over the next 48 hours let's proceed.

@kenjis
Copy link
Member Author

kenjis commented Apr 12, 2022

More than 48 hours have passed. We proceed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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
3 participants