isSameUnit not working like in previous versions and accepting "null" as argument about carbon

[glamorous]

I just tested it with v3.x-dev an there it works (again)! But at dev-master it's broken.

[kylekatarnls]

Hello,

I will further check if it's expected or if it's the PHPDoc that is wrong.

But in any case I strongly recommend not to do that.

Doing Carbon::parse('2018-06-01')->isSameDay(null) . It's kinda like doing "orange" + 8 => trying to do an operation with different things that has no particular reason to mean anything together. There is no reason why null should be comparable to any kind of date.

You should pass to isSameDay() only a DateTimeInterface or a valid string representing a date-time.

If you mean to say "is same day as now", just use:

Carbon::parse('2018-06-01')->isToday()

Instead, it will make your code less ambigous.

[kylekatarnls]

Hello, I confirm this is expected since PHPDoc types were turned into concrete type: c650829

All the isSame* method will now only accept non-option DateTimeInterface|string $date parameter.

I will align the PHPDoc.

[kylekatarnls]

Documentation fixed in e7c9d37

[glamorous]

Hello, I confirm this is expected since PHPDoc types were turned into concrete type: c650829

All the isSame* method will now only accept non-option DateTimeInterface|string $date parameter.

I will align the PHPDoc.

Don't want to offend you, but that's a breaking change since Carbon 2.x version and has nothing to do with the strict type change because in the commit you've shared, In the referenced commit the change was made to have al fallback for isNext (for passing null), but the same fallback wasn't added to isSame

c650829#diff-ede373ba22ef24bd43ba5f8bbc290b90d23d78caf63b8c75cb0971534df3da4bR2707

So changing the docs will indeed align the docs with the behaviour, but that will make a breaking change and in my opion nothing to do with "strict types"

[kylekatarnls]

Coming from weaker types (Some Carbon method were crafted with PHP 5 support), we have false and null getting accepted just by accident. I wouldn't even say "working" because Carbon::parse('2018-06-01')->isSameDay(null) === true just doesn't make any sense to be fair.

3.x-dev should be now aligned on dev-master, it was a mistake that it was not updated yet.

This change should apply as early as 3.0.0 (stable) if I'm not mistaken so it's indeed a breaking change, but that's the purpose of bumping major version number, it's to actually do breaking change to fix nonsenses like this one.

[glamorous]

To my defense it was used like this:

$formatPattern = $this->start_initial->isSameDay($this->end_initial) ? 'H:i' : 'd/m H:i';

Where my workaround/fix now is:

$formatPattern = $this->start_initial->isSameDay($this->end_initial ?? now()) ? 'H:i' : 'd/m H:i'

because the end_initial is a Carbon but can also be null. So because it was supported in 2.x I assumed i could just use it off course. The workaround works too, but feels a bit more "ugly" 🤷

[kylekatarnls]

No, it's not ugly at all! That's way cleaner because in the first code you have a completely non obvious implicit knowledge that $this->end_initial === null means "ending now". Other business cases could give to null a completely different meaning (such as "never ends", "end not yet decided or calculated", "end not known")

For the record $this->end_initial ?? 'now' would also work as the functions accepts string representing date-time.

With your new code, new developers and your future you no longer have to guess and remember this implicit rule as it's made explicit.

You can find a lot of articles telling you why returning null or giving a specific meaning to null values is typically considered like a bad practices for many other reasons. And comparing null to another value is one of those code smells which does not just apply to dates but also to any other objects or scalar values.