[File Uploads] Check if the entity contains the files #7358
Conversation
After many tests, it seems that if the entity does not contains the files, the event throw a Exception, by checking the presence of files, the exception isn't thrown and the process can continue, this can be usefull if the entity isn't linked with file or if the file was stored into an array but not needed at this time. The return isn't needed after the if statement.
|
|
||
| $entity->setBrochure(new File($this->targetPath.'/'.$fileName)); | ||
|
|
||
| // Only for tests purpose and proper object validation |
There was a problem hiding this comment.
Checking for the right object below indeed is needed. However, this comment looks confusing to me and I would just remove it.
| $entity->setBrochure(new File($this->targetPath.'/'.$fileName)); | ||
|
|
||
| // Only for tests purpose and proper object validation | ||
| if (!$entity instanceof ObjectWanted) { |
There was a problem hiding this comment.
the class should be Product here
|
Hi @xabbuh, Just make the correction with a new commit, should be better, for the verification, my bad, i don't look at the entity checked before. |
|
@Guikingone Thanks for the fast reaction. If you could do one minor change, this one looks ready for me to be merged: Can you please update the code to always use four spaces for indentation? |
|
@xabbuh Done, sorry for the bad identation. |
|
👍 Thanks for this nice fix! Status: Reviewed |
| } | ||
|
|
||
| if ($entity->getBrochure()) { | ||
| $fileName = $entity->getBrochure(); |
There was a problem hiding this comment.
You can move this initialization into the if to avoid duplicating the call.
There was a problem hiding this comment.
Don't really understand this movement, the first if check if the entity passed is the right instance, the second if is launched only if the first pass ...
If i move the second into the first, that change the whole condition because its suggest that my entity has file (who's not always the case) ...
Or maybe, you suggest something like that :
if (!$entity instanceof Product) {
return;
} elseif ($entity->getBrochure()) {
$fileName = $entity->getBrochure();
$entity->setBrochure(new File($this->targetPath.'/'.$fileName));
}Not sure about the movement in fact ...
There was a problem hiding this comment.
Sorry if I wasn't clear, I mean you can write:
if ($fileName = $entity->getBrochure()) {
$entity->setBrochure(new File($this->targetPath.'/'.$fileName));
}There was a problem hiding this comment.
Oh, see want you thinking about, in this case, yes, that work but let's imagine we have an array of files, with this approach, the array must be read before the instantiation of File, let's imagine the case of an article with multiples images linked :
/**
* @param LifecycleEventArgs $args
*
* @throws FileNotFoundException
*/
public function postLoad(LifecycleEventArgs $args)
{
$entity = $args->getObject();
if (!$entity instanceof Article) {
return;
}
// Only if the entity has store files.
if ($entity->getImages()) {
$filename = $entity->getImages();
foreach ($filename as $file) {
$entity->addImage(new File($this->imagesDir.'/'.$file));
}
}
}In this particular case, we must read every every entry and check if the entity return any entry into the array, this way, $filename become an array of images and we can instantiate every images into a file, i know, that's a particular case but my vision was about thinking this type of case.
And yes, we can iterate about the method return :
/**
* @param LifecycleEventArgs $args
*
* @throws FileNotFoundException
*/
public function postLoad(LifecycleEventArgs $args)
{
$entity = $args->getObject();
if (!$entity instanceof Article) {
return;
}
// Only if the entity has store files.
if ($entity->getImages()) {
foreach ($entity->getImages() as $image) {
$entity->addImage(new File($this->imagesDir.'/'.$image));
}
}
}But i find the first vision cleaner and better for future check.
There was a problem hiding this comment.
The same thing applies for an array ;)
if ($images = $entity->getImages()) {
foreach ($images as $image) {
$entity->addImage(new File($this->imagesDir.'/'.$image));
}
}There was a problem hiding this comment.
Yes, not wrong, gonna make the correction into the file.
|
@HeahDude Should be better :) |
|
Thank you @Guikingone. |
…ikingone) This PR was submitted for the master branch but it was merged into the 2.7 branch instead (closes #7358). Discussion ---------- [File Uploads] Check if the entity contains the files After many tests, it seems that if the entity does not contains the files, the event throw a Exception, by checking the presence of files, the exception isn't thrown and the process can continue, this can be usefull if the entity isn't linked with file or if the file was stored into an array but not needed at this time. The return isn't needed after the if statement. Commits ------- cc0d418 [File Uploads] Check if the entity contains the files
After many tests, it seems that if the entity does not contains the files, the event throw a Exception, by checking the presence of files, the exception isn't thrown and the process can continue, this can be usefull if the entity isn't linked with file or if the file was stored into an array but not needed at this time.
The return isn't needed after the if statement.