You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
So I've been using the image upload feature, following the documentation and I was thinking there is maybe room for improvement, specifically on the model mutator end of things.
Here is my concern: say you have an image upload for a model called Article that has an image attribute that is required, and not nullable is the database.
Here would be the code snippet for that attributes mutator (exactly as in the docs)
publicfunctionsetImageAttribute($value)
{
$attribute_name = "image";
$disk = "public";
$destination_path = "folder_1/subfolder_1";
$this->uploadFileToDisk($value, $attribute_name, $disk, $destination_path);
// return $this->attributes[{$attribute_name}]; // uncomment if this is a translatable field
}
This would in practice work fine if the form validation makes the field required.
But say now you want to test your Article model. You would have a test that looks like this (assuming the factory does in fact correctly set a random value for the image attribute):
/** * Test if this model saves properly to database * * @return void */publicfunctiontest_creates_properly()
{
Article::factory()->count(5)->create();
$this->assertDatabaseCount('articles', 5);
}
Aaaaand your test would fail. This is because the mutator prevents the image attribute to be set, as there is no image in the request().
Now there is a solution that is quite simple:
publicfunctionsetImageAttribute($value)
{
$attribute_name = "image";
$disk = "public";
$destination_path = "folder_1/subfolder_1";
$this->uploadFileToDisk($value, $attribute_name, $disk, $destination_path);
// After all checks have been done, set the attribute anyway if there is no file and value is set// This is because we need the factory to function properly in unit tests without a Request file// Form Validation will have the responsibility to check file uploadif (!request()->hasFile($attribute_name) && !is_null($value)) {
$this->attributes[$attribute_name] = $value;
}
// return $this->attributes[{$attribute_name}]; // uncomment if this is a translatable field
}
Now I am wondering: is this the right solution ? This assumes that checking that there is an actual image is the forms validations responsibility.
It also assumes that the unit test should pass as it is, as we are not testing a file upload feature but simply if a model creation works.
Another thought might be that these request() checks and, in fact, the whole file upload feature shouldn't be the models responsibilty, but rather of the Controller.
So i'm figuring that if the solution is correct you guys might wan't to update the docs, and if not let's talk about better solutions :) .
reacted with thumbs up emoji reacted with thumbs down emoji reacted with laugh emoji reacted with hooray emoji reacted with confused emoji reacted with heart emoji reacted with rocket emoji reacted with eyes emoji
Uh oh!
There was an error while loading. Please reload this page.
-
Hi !
So I've been using the image upload feature, following the documentation and I was thinking there is maybe room for improvement, specifically on the model mutator end of things.
Here is my concern: say you have an image upload for a model called
Article
that has animage
attribute that is required, and not nullable is the database.Here would be the code snippet for that attributes mutator (exactly as in the docs)
This would in practice work fine if the form validation makes the field required.
But say now you want to test your Article model. You would have a test that looks like this (assuming the factory does in fact correctly set a random value for the
image
attribute):Aaaaand your test would fail. This is because the mutator prevents the
image
attribute to be set, as there is no image in therequest()
.Now there is a solution that is quite simple:
Now I am wondering: is this the right solution ? This assumes that checking that there is an actual image is the forms validations responsibility.
It also assumes that the unit test should pass as it is, as we are not testing a file upload feature but simply if a model creation works.
Another thought might be that these
request()
checks and, in fact, the whole file upload feature shouldn't be the models responsibilty, but rather of the Controller.So i'm figuring that if the solution is correct you guys might wan't to update the docs, and if not let's talk about better solutions :) .
Cheers
Beta Was this translation helpful? Give feedback.
All reactions