-
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
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!
* @mixin Eloquent | ||
* @method static Builder create(...$values) | ||
* @method static Builder truncate() | ||
* @method static Eloquent sole(...$parameters) |
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
@saineshmamgain can you look at the test failure:
|
@GromNaN actually this error comes when I do the reset |
@GromNaN My bad missed call to |
Thank you @saineshmamgain |
This PR fixes the issue #3341 .
Basically when
Date::use(CarbonImmutable::class);
is added. The methodasDateTime
inDocumentModel
trait throws an exception. I've changed the return type ofasDateTime
method fromCarbon
toDateTimeInterface
.Checklist