Skip to content

fix: make Time::__toString() database-compatible on any locale #6461

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

Merged
merged 8 commits into from
Sep 7, 2022

Conversation

kenjis
Copy link
Member

@kenjis kenjis commented Aug 31, 2022

Description
Closes #6439
Supersedes #6456

In this PR, Time::__toString() no longer depends on locale.
It always returns database-compatible string like '2016-03-09 12:00:00'.

In the current implementation, the __toString() output will be changed by the locale.
(But it seems it is not documented in the User Guide.)
It causes database query error in a few locale.

Time is localized class, but __toString() does not need to return localized value.
If a dev needs localized string, they can call toDateTimeString().

Implicitly changing behavior depending on locale is a bad practice because it confuses developers.
__toString() is a standard PHP method, not a localized helper method.
The return value should be database-compatible.

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 added bug Verified issues on the current code behavior or pull requests that will fix them breaking change Pull requests that may break existing functionalities labels Aug 31, 2022
@MGatner
Copy link
Member

MGatner commented Sep 6, 2022

I think this is the way to go. Since that format is very clearly "database DateTime" I expect the intent was always to have the Stringable be database-compatible.

@kenjis kenjis changed the title fix: Time::__toString() depends on locale fix: make Time::__toString() database-compatible on any locale Sep 7, 2022
setUp()/tearDown() take care of it.
Co-authored-by: Pooya Parsa Dadashi <[email protected]>
@kenjis
Copy link
Member Author

kenjis commented Sep 7, 2022

Added the docs.

kenjis and others added 2 commits September 7, 2022 14:55
Co-authored-by: John Paul E. Balandan, CPA <[email protected]>
Co-authored-by: John Paul E. Balandan, CPA <[email protected]>
@kenjis kenjis requested a review from datamweb September 7, 2022 09:20
Copy link
Contributor

@datamweb datamweb left a comment

Choose a reason for hiding this comment

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

Kenjis, I know you have sent several PRs to solve this problem and finally here we are.
It works fine now.
Thanks.

@kenjis kenjis merged commit 2accd78 into codeigniter4:develop Sep 7, 2022
@kenjis kenjis deleted the fix-Time-toString branch September 7, 2022 21:53
@kenjis
Copy link
Member Author

kenjis commented Sep 7, 2022

Thank you all!

@kenjis
Copy link
Member Author

kenjis commented Sep 7, 2022

🎉
I think this will prevent us from producing broken code that causes a database error only if the locale is fa or ar.

kenjis added a commit to kenjis/CodeIgniter4 that referenced this pull request Sep 8, 2022
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.

make Time object usable with QueryBuilder in all locales
4 participants