-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Add model property types to magic where methods. #989
Open
jpickwell
wants to merge
2
commits into
barryvdh:master
Choose a base branch
from
jpickwell:patch-1
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 mention this in my previous feedback but it might just have gotten lost: any method accepting Carbon (or a variation thereof) also accept strings, it doesn't just accept an instance (or null).
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.
Understood, but if you look at the created_at property, it's also
\Carbon\CarbonImmutable|null
, so this is not an issue with this new feature.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.
The
whereProperty
method parameter type is now exactly the same as the corresponding@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 pointed this out in #989 (review)
However this assumption isn't correct.
Let's look at this tinker session:
Setting either string or Carbon is fine.
However, in the IDE:
I noticed something else in a private project I tested this:
Migration:
Model:
Before:
After:
I think
string
is practical in this context, there's not much point about this anyway as it's a JSON column.But then I've another model with a similar column which however generated this:
Migration:
Model:
The
mixed
is in fact also not that bad here, actually might make even more sense.Though I could not figure out why one is
string
here and one ismixed
. I tried removing the set/get but it didn't change 🤷♀️Whilst I'm not yet in the clear regarding the latter new examples, I think that date casts not accepting string is bad. The
laravel-ide-helper
is supposed to help but would get in the way here.These were only easy examples with
$date
andarray
casts, but maybe properties in$casts
have to be handled differently?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'm not disputing the string issue with dates. However, that's an existing issue in
laravel-ide-helper
. You have the same issue with setting the magic properties. I'm not going to fix this issue in this PR. That's outside the scope of the PR. If you want that fixed, then create an issue and maybe someone will create a PR to rectify the issue.If you look at the
CustomDate
snapshot, you'll see that the@property
tags and the correspondingwhere
@method
tags have the same value type:The type for the
$value
parameter is exactly the same as the corresponding@property
type. And, when I say "exactly the same", I mean that the string is calculated using the same helper method in the command class, see the following lines in theModelsCommand.php
file in this PR:482
-- getting the initial type for a property487
-- passing the type to thesetProperty
method495
-- getting the "true" type for the property for thewhere
method by callinggetTruePropertyType
728
-- same method call, but for the property itself822
-- shows that no other type manipulation is performed when generating the PhpDoc@property
tagsThere 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.
That's a pretty strong feedback here, not sure where this is coming from.
It made me think though whether this whole feature should be optional (opt-in/out => debatable)
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.
Using the example of dates as
\Carbon\CarbonImmutable
and not allowingstring
s is an issue that already exists in the code base. I did not introduce this issue. If you look at the diffs for the test snapshots, you'll see that the@property
tags did not change.My objection to your feedback is that you're asking me to fix an issue, an issue that already existed, when I'm trying to introduce a new feature. If this existing issue stood in the way of introducing this new feature, then I would fix it. However, the issue does not hinder this feature's implementation. Clearly, what this feature does do is highlight the existing issue. I agree the issue should be fixed, but I believe that it should be fixed in a new PR, one that targets the issue specifically.
I don't have any objection to adding a flag for this feature.
Just to be extra clear, to reiterate what I've been saying, the
@property
tags have not been affected by my changes.Here's what the CustomDate test snapshot looks like before my changes:
Notice that the
$created_at
and$updated_at
properties have the same issue you're asking my to fix. I feel like you're ignoring this fact, and just assuming that somehow I have introduced this issue when in reality I have not.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 created a new PR (#1056) to fix the type annotations for date attributes/properties.
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.
One more thing: looking at the JSON example you gave, I will try to fix that one. That has to do with the command not checking casts until after getting properties from the table:
castPropertiesType
should probably be called fromgetPropertiesFromTable
.