Closed sshead closed 3 months ago
Hey @sshead! Sorry for the slow response on this one - this week's been crazy busy haha! I'm intending on checking this PR out tonight or tomorrow and getting back to you on it 😄
Hey @sshead, huge thanks for this fix and the test update too! I'd have only done pretty much the same thing myself 😄
The Carbon
, CarbonInterface
, and CarbonImmutable
topic is something I'm definitely going to think about when I do the next major version. This works at the moment (and is the quickest/easiest to get released), but I reckon I'd prefer to use an approach like you proposed in your last PR in the long run. Maybe we can reopen that PR at some point in the future.
Thanks again, I appreciate it! 🔥
Replaces #284.
Recap: Currently the package breaks if your app has configured the
Date
facade to useCarbonImmutable
instead ofCarbon
(as per this blog post by Michael Dyrynda).To get it working is a one-line fix - changing one instance of
now()
toCarbon::now()
in theBuilder
class.Some comments/caveats:
datetime
properties on Eloquent models contain an object of the correct class (eitherCarbon
orCarbonImmutable
, depending on how you've configured theDate
facade), no matter what was used to set them.ShortURL
andShortURLVisit
models are incorrect: all references toCarbon
there should beCarbon|CarbonImmutable
. Would changing those doc blocks be a breaking change? Otherwise I can change those as well.CarbonImmutable
... not sure that's worth doing!Hope this is an improvement on the last effort.