Importer tweaks #14800
Importer tweaks #14800
Conversation
PR Summary
|
There was a problem hiding this comment.
I haven't done a full review of this but I'm going to "request changes" just to get the first questions posted and also say that the addition of the accessors/mutators probably warrants checking the places they are used throughout the codebase to make sure we're not causing any unexpected issues. I'm thinking we might be double-parsing a date here or there now which shouldn't cause an issue but should still be double-checked.
app/Models/Asset.php
Outdated
| public function getLastCheckoutAttribute($value) | ||
| { | ||
| return $this->attributes['last_checkout'] = $value ? Carbon::parse($value)->format('Y-m-d H:i:s') : null; | ||
| } | ||
|
|
||
| public function setLastCheckoutAttribute($value) | ||
| { | ||
| $this->attributes['last_checkout'] = $value ? Carbon::parse($value)->format('Y-m-d H:i:s') : null; | ||
| } | ||
|
|
||
| public function getLastCheckinAttribute($value) | ||
| { | ||
| return $this->attributes['last_checkin'] = $value ? Carbon::parse($value)->format('Y-m-d H:i:s') : null; | ||
| } | ||
|
|
||
| public function setLastCheckinAttribute($value) | ||
| { | ||
| $this->attributes['last_checkin'] = $value ? Carbon::parse($value)->format('Y-m-d H:i:s') : null; | ||
| } | ||
|
|
||
| public function getLastAuditDateAttribute($value) | ||
| { | ||
| return $this->attributes['last_audit_date'] = $value ? Carbon::parse($value)->format('Y-m-d H:i:s') : null; | ||
| } | ||
|
|
||
| public function setLastAuditDateAttribute($value) | ||
| { | ||
| $this->attributes['last_audit_date'] = $value ? Carbon::parse($value)->format('Y-m-d H:i:s') : null; | ||
| } | ||
|
|
||
| public function getAssetEolDateAttribute($value) | ||
| { | ||
| return $this->attributes['asset_eol_date'] = $value ? Carbon::parse($value)->format('Y-m-d') : null; | ||
| } | ||
|
|
||
| public function setAssetEolDateAttribute($value) | ||
| { | ||
| $this->attributes['asset_eol_date'] = $value ? Carbon::parse($value)->format('Y-m-d') : null; | ||
| } |
There was a problem hiding this comment.
If this gets merged we should remove these fields from $casts since we're overwriting the functionality. I wouldn't be surprised if keeping them in $casts while additionally handling the accessors and mutators ourselves causes a bug down the line because the framework (and/or libraries) assume it is in control of casting the included properties.
Additionally, if this is getting merged, you may want to consider using the new accessor/mutator syntax that groups them into one method for each field so it is easier to read.
There was a problem hiding this comment.
Casting has never worked the way we expected anyway tbh, since it only kicks in when the data is serialized IIRC. It's why we've gone back and forth so many times :(
There was a problem hiding this comment.
Dates have especially always behaved weirdly in the importer (since that ignores the casting), and I can't help but think that using accessors and mutators is the most consistent path.
There was a problem hiding this comment.
I really wish we wouldn't do this. A few reasons:
- Framework expectations, 90% of the time I'm grabbing a date from a model I'm expecting it to be cast so that I have carbon already available. I'd love to see us work with the framework more often than against it.
We should follow framework convention as much as we possibly can - I think that's what will help us iterate more quickly and possibly even see more involvement from the FOSS community. - The getters I feel are actively or have the potential to be actively breaking casting, as Marcus said above.
- If in the future we want to use a date-time-picker a little more tightly integrated it likely wouldn't work with this out of the box (think something like filament forms: https://filamentphp.com/docs/3.x/forms/fields/date-time-picker)
- On the setter front, with casts, as long as we receive something that's parsable from Carbon we can save it directly to the model instead of having to format it manually.
Casting has never worked the way we expected anyway tbh, since it only kicks in when the data is serialized IIRC. It's why we've gone back and forth so many times :(
It's cast as a Carbon object, then the formatting kicks in when an object doesn't make sense (when it's serialized).
There was a problem hiding this comment.
Casting has never worked the way we expected anyway tbh, since it only kicks in when the data is serialized IIRC
I was caught off guard by this but it makes sense to me now: you get all the niceties of a carbon object without having to do the casting yourself. So we can compare against other objects or format differently while the model is being passed around the application (etc) and then it falls back to default formatting when leaving the application layer.
Overall, I agree with Spencer's points and think we should lean into the framework as much as possible.
There was a problem hiding this comment.
The getters I feel are actively or have the potential to be actively breaking casting, as Marcus said above.
The casting has literally NEVER worked. It's only invoked in JSON and serialization, and we already use a formatting helper for that.
On the setter front, with casts, as long as we receive something that's parsable from Carbon we can save it directly to the model instead of having to format it manually.
This is literally not what my own tests with this PR have shown. Please, by all means, prove me wrong. Regardless of how I tried to parse the date, if it was blank, it came through as 0000-00-00 or 0000-00-00 00:00:00, even while checking for nulls, try/catching whether or not Carbon could parse it, etc.
If I was missing something, I'm glad to take the correction, but this has never worked the way we wanted/expected, and I need stuff that actually works, versus what's "correct".
There was a problem hiding this comment.
So we can compare against other objects or format differently while the model is being passed around the application (etc) and then it falls back to default formatting when leaving the application layer.
@marcusmoore The only time we really compare dates, we've already got helpers for that (since we always want to display the date according to the user's own preferences in the settings.) Otherwise, it's not a carbon object, just a date or date time field in the database and we're comparing those at a SQL level.
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
I have no idea why this is necessary Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
snipe
force-pushed
the
fixes/importer_tweaks
branch
from
65e2cdd
to
1cdec61
Compare
|
If one of the two of you want to take a crack at this yourself, I'm happy to retire this PR, but I don't make decisions like mutators etc lightly. If you can make it work without them, go nuts. |
I'd like to give it a go but don't want to hold up a legitimate bug fix. Maybe we just go ahead with this PR as is and me/Spencer can take a swing at it afterwards? |
This PR:
$defaultStatusLabelIdcould be a non-deployable status typeI know we've gone back and forth on the getters/setters stuff with dates, but this makes handling bad data in imports much easier. I'm expecting a fight tho ;)