-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Fixes: getting immutable_datetime property fails if Date::use(CarbonImmutable::class) is set
#3342
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
Fixes: getting immutable_datetime property fails if Date::use(CarbonImmutable::class) is set
#3342
Conversation
...setting `Date::use(CarbonImmutable::class);`
...able-property' into fix/error-accessing-carbon-immutable-property
...able-property' into fix/error-accessing-carbon-immutable-property
...able-property' into fix/error-accessing-carbon-immutable-property
@GromNaN
GromNaN
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a minor comment, but the overall seems good to me. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These annotations are interesting, I think we should document this practice (something I will ask the doc team).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
credit goes to @Treggats. I followed his HiddenAnimal model.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy to help :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@GromNaN the thing with Laravel Models is that by default they have the type Model. Which makes sense, as the model extends that class, but then the IDE doesn't know about any of the methods, relations, properties, etc.
These docblocks provide the IDE with the context it should have. Note; the @method docblock can not be used if the method already exists in the class.
Personally I also use @property for the columns of the database table and the relation once fetched.
The @property-read docblock is used for the Builder return type of a relation.
A little gotcha is that sometimes you need a additional docblock on the variable. Like this.
use Tests\Models\Anniversary; /** @var Anniversary $anniversary */ $anniversary = Anniversary::query()->where('name', 'John')->first();
It then knows about the properties, relations, etc.
Hope this helps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I'm tracking this to work on it later PHPORM-316
GromNaN
commented
Apr 10, 2025
@saineshmamgain can you look at the test failure:
1) MongoDB\Laravel\Tests\Eloquent\DateTimeImmutableTest::testCanReturnCarbonImmutableObject
* Test code or tested code did not remove its own error handlers
* Test code or tested code did not remove its own exception handlers
/home/runner/work/laravel-mongodb/laravel-mongodb/tests/DateTimeImmutableTest.php:28
saineshmamgain
commented
Apr 10, 2025
@GromNaN actually this error comes when I do the reset Date::useDefault(); in tearDown Method.
saineshmamgain
commented
Apr 10, 2025
@GromNaN My bad missed call to parent::tearDown() in my method. Updated the PR.
GromNaN
commented
Apr 10, 2025
Thank you @saineshmamgain
This PR fixes the issue #3341 .
Basically when
Date::use(CarbonImmutable::class);is added. The methodasDateTimeinDocumentModeltrait throws an exception. I've changed the return type ofasDateTimemethod fromCarbontoDateTimeInterface.Checklist