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: Time::setTestNow() does not work with fa Locale #6116

Merged
merged 1 commit into from
Jun 14, 2022

Conversation

kenjis
Copy link
Member

@kenjis kenjis commented Jun 13, 2022

Description

ErrorException : DateTime::modify(): Failed to parse time string (۲۰۱۷-۰۳-۱۰ ۰۰:۰۰:۰۰) at position 0 (�): Unexpected character
 .../CodeIgniter4/system/I18n/Time.php:96

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 the bug Verified issues on the current code behavior or pull requests that will fix them label Jun 13, 2022
@kenjis kenjis force-pushed the fix-Time-setTestNow-bug branch from 5d2e49c to 457d5ec Compare June 13, 2022 09:54
@MGatner
Copy link
Member

MGatner commented Jun 13, 2022

I'm not sure what is happening with Coveralls. Maybe it is getting confused about the different branches?

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.

Actually now I'm not sure about this. The User Guide is very clear that toLocalizedString() is for localization whereas toDateTimeString() "will return a string formatted as you would commonly use for datetime columns in a database (Y-m-d H:i:s)" (https://codeigniter4.github.io/userguide/libraries/time.html#todatetimestring).

Reading that makes me think the problem is actually using the localization in toDateTimeString().

@kenjis
Copy link
Member Author

kenjis commented Jun 13, 2022

It is difficult to read but I think toDateTimeString() is localized version of Y-m-d H:i:s.

This is the first of three helper methods to work with the IntlDateFormatter without having to remember their values. This will return a string formatted as you would commonly use for datetime columns in a database (Y-m-d H:i:s):
https://codeigniter4.github.io/CodeIgniter4/libraries/time.html?highlight=time#todatetimestring

It is clear if you see the implementation.

@MGatner
Copy link
Member

MGatner commented Jun 13, 2022

You're right! But the UG is still confusing: I would have used toDateTimeString() here as well. I wonder if we need a new method specifically for non-localized database values? Is there ever a case where your timestamp values need to be localized for storage input?

@MGatner
Copy link
Member

MGatner commented Jun 13, 2022

After some reading it looks like DATETIME/TIMESTAMP inputs must always be decimal. I think we need to add toDatabase() or something like it and update the User Guide so developers aren't accidentally using localized strings for database input.

@datamweb
Copy link
Contributor

Friends just for information, the output of command toLocalizedString() is also UTF-8 string (33) "۲۰۲۲-۰۶-۱۳ ۱۷:۳۰:۱۳".

@paulbalandan
Copy link
Member

Is this related to here?
#4708

@kenjis
Copy link
Member Author

kenjis commented Jun 14, 2022

@paulbalandan Yes. The issue is because of the same bug.

ErrorException : DateTime::modify(): Failed to parse time string (۲۰۱۷-۰۳-۱۰ ۰۰:۰۰:۰۰) at position 0 (�): Unexpected character
 .../CodeIgniter4/system/I18n/Time.php:96
@kenjis
Copy link
Member Author

kenjis commented Jun 14, 2022

But the UG is still confusing

I sent a PR #6121

@kenjis kenjis mentioned this pull request Jun 14, 2022
3 tasks
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.

Okay, I am on board - this is definitely e right direction. 4.3 feature request: toDatabase()

@kenjis kenjis merged commit 8e5360c into codeigniter4:develop Jun 14, 2022
@kenjis kenjis deleted the fix-Time-setTestNow-bug branch June 14, 2022 12:04
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.

5 participants