-
Notifications
You must be signed in to change notification settings - Fork 14
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
fix: insert method with fillable of the model #178
Conversation
src/Traits/EntityControlTrait.php
Outdated
array_walk($timestamps, function (&$timestamp) { | ||
$timestamp = $timestamp instanceof Carbon | ||
? $timestamp | ||
: Carbon::parse($timestamp); | ||
}); |
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.
as discussed, no need to map data into Carbon instance, developer should fill timestamps with the correct data type in case he fill timestamps manually
src/Traits/EntityControlTrait.php
Outdated
$timestamps = array_merge($defaultTimestamps, Arr::only($fillableFields, [ | ||
$this->model::CREATED_AT, | ||
$this->model::UPDATED_AT | ||
])); | ||
|
||
return array_merge($fillableFields, $timestamps); |
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.
let's try to make something like this
$timestamps = array_merge($defaultTimestamps, Arr::only($fillableFields, [ | |
$this->model::CREATED_AT, | |
$this->model::UPDATED_AT | |
])); | |
return array_merge($fillableFields, $timestamps); | |
return array_merge($timestamps, $fillableFields); |
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.
@yogyrton please check again, it should works fine
tests/EntityControlTraitTest.php
Outdated
@@ -16,6 +16,7 @@ class EntityControlTraitTest extends HelpersTestCase | |||
use SqlMockTrait; | |||
|
|||
protected string $mockedNow = '2020-01-01 00:00:00'; | |||
protected string $mockedFillableDate; |
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.
is it required?
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.
Yes
We use this in the test . In the model $timestamps = true, $fillable = ['creation_date'] and we check that the creation_date field is written to the database with our value, and the updated_date field is the default
src/Traits/EntityControlTrait.php
Outdated
$timestamps = array_merge($defaultTimestamps, Arr::only($fillableFields, [ | ||
$this->model::CREATED_AT, | ||
$this->model::UPDATED_AT | ||
])); | ||
|
||
return array_merge($fillableFields, $timestamps); |
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.
@yogyrton please check again, it should works fine
# Conflicts: # src/Traits/EntityControlTrait.php
@yogyrton please resolve conflicts |
# Conflicts: # tests/support/Mock/Models/TestModel.php
$this->mockedNow, 'test_name_1', $this->mockedNow, | ||
$this->mockedNow, 'test_name_2', $this->mockedNow, | ||
$this->mockedNow, 'test_name_3', $this->mockedNow, | ||
$this->mockedFillableDate, 'test_name_1', $this->mockedNow, |
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.
as discussed, please just hardcode this value instead if using class property
#165 (comment)