Closed sshead closed 3 months ago
Hey @sshead, thanks for the PR! 😄
When I first built this package, I was still kinda new to everything, so there's some decisions I made (because I didn't know any better) than I definitely wouldn't do now. One of them being, I would have encouraged the use of interfaces and/or immutable dates instead of directly using the Carbon\Carbon
class in the signature like that.
So I love the idea behind this change! But I'm wondering if there's a way for us to do this without changing the method signature and causing a breaking change? 🤔
Oh, it never occurred to me that any change to a method signature would be a breaking change. Is that the case even if you're broadening the types of a parameter? With inheritance this is fine (contravariance) ... but as you can tell, I'm not a package developer!
Anyway, I think there is another way: Simply change some instances of the now()
Laravel helper to Carbon::now()
- in particular, where an assignment is made to a model field. For example, change:
$this->activateAt = now();
to:
$this->activateAt = Carbon::now();
I've tried it locally and it seems to work - tests pass, and it doesn't break my app that uses CarbonImmutable
by default. Would you like me to submit a new PR with that solution?
OK, so deep dive was well worth it! It turns out that it's a one-line fix, at least to get it working with an app that uses CarbonImmutable
. I've submitted a new PR (#285), and I'll close this one.
The method signatures that I changed previously - for activateAt()
and deactivateAt()
on the Builder
class - are fine to leave. At worst, they may be slightly inconvenient or annoying if you've configured your app to use CarbonImmutable
, since you'd have to manually pass in the right thing (e.g. using Carbon::now()
instead of the global now()
helper). But that's very minor.
I love this package, but it currently breaks in my app, because in my Laravel apps I prefer to set the
Date
facade to useCarbonImmutable
instead ofCarbon
(as per this excellent blog post by my fellow Aussie Michael Dyrynda).This PR fixes that. The only things I had to change were:
Carbon|CarbonImmutable
rather than justCarbon
. This applies toactivated_at
,deactivated_at
,created_at
andupdated_at
.Date::now()
rather thanCarbon::now()
(only in ShortURLVisitFactory.php).I've added tests as well - hope I didn't go overboard with those! If there's a better way of testing, let me know. (And if I've butchered this PR completely, apologies - I'm a spare-time hobbyist dev, so I very rarely attempt these.)