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

Carbon parse behaviour differs from native DateTime #3113

Closed
quarkcore opened this issue Nov 26, 2024 · 3 comments
Closed

Carbon parse behaviour differs from native DateTime #3113

quarkcore opened this issue Nov 26, 2024 · 3 comments

Comments

@quarkcore
Copy link

Hello,

I encountered an issue with the following code:

$t1 = Carbon::parse(false);
// output $t1
// 1970-01-01 00:00:00.0 +00:00,                                                                                                                                               

$2 = Carbon::parse(true);
// output $t2
// 1970-01-01 00:00:01.0 +00:00

Carbon version: 3.8.0

PHP version: PHP 8.3.13

I expected to get:

Native PHP DateTime gives me:

For false:

new DateTime(false); // 2024-11-26 09:00:00.000001

For true:

new DateTime(true); // DateMalformedStringException

But I actually get:

$t1 = Carbon::parse(false);
// output $t1
// 1970-01-01 00:00:00.0 +00:00,                                                                                                                                               

$2 = Carbon::parse(true);
// output $t2
// 1970-01-01 00:00:01.0 +00:00

This was executed inside a docker container which runs a laravel app (therefore carbon is installed as dep of laravel/framwork v11.29.0)

I prosposed a PR with a change that would reflect the behaviour of PHP native DateTime: #3112

Thanks!

@kylekatarnls
Copy link
Collaborator

kylekatarnls commented Nov 27, 2024

Hello, 👋 thanks for the report.

Carbon does not differ from DateTime in this regard since new Carbon(false) creates a Carbon instance.

DateTime::parse() does not exist so there is no need for Carbon::parse() to be compatible with something from DateTime

And creating a date from false makes no sense, it's allowed in DateTime (and so Carbon) constructor for legacy reason. There is nothing relevantly linking false to Jan 1st 1970 and even less true to Jan 1st 1970 + 1 second. This is just an artifact of conversion to integer + interpreting integer as a timestamp.

Library should not open door to ambiguity by relaxing method to meaningless types.

If this is what you mean then you should explicitly cast your value with (int).

@quarkcore
Copy link
Author

quarkcore commented Nov 27, 2024

I stumbled across the behaviours revisiting some Eloquent ORM code which made use of Carbon in a where clause to query the db. Would have expected that Carbon::parse(bool $value) either returns null or throws.
For now PHPs type juggling does its "job" there and converts bool to something even if bool is not an allowed type by signature of Carbon::parse(). But yes, you could have a point that this is not something to be covered inside a date utility like Carbon.

@kylekatarnls
Copy link
Collaborator

Hello, I cannot find Carbon::parse in Eloquent source code: https://github.com/search?q=repo%3Ailluminate%2Fdatabase%20Carbon%3A%3Aparse&type=code

Did you mean code using Eloquent in your app?

If you found code that is calling Carbon::parse(false) in a library you can point me to this code?

Either app or third-party, recommendation would be the same:

  • Identify the case that make the value being false
  • Challenge if it should really have been accepted by the current layer
    • If not, either throw an exception to be handled by higher level, or sanitize the value, or validate it at the system input.
  • If it really has to be allowed, identify the meaning of it in this context and early-return an explicit value so you skip entirely Carbon:parse()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants