Moving Uploaded File

One thing I can see

'description' => $request->get('description', null),

Thanks, @bashy...

That doesn't actually work though. Because description is a textarea in the form, it actually sets it to an empty string, rather than null, which means an empty string gets persisted in the database.

Creating a file (or just generating a filename) shouldn't be something controller should know much about, so you could have a dedicated command for it, if you really want to make your code nicer.

Think of controllers as web-browser specific approach to the app. But you might also want to create a file or generate a filename from your command line or somewhere else in your app. Would you copy&paste the logic behind it, or have one place (a dedicated command) for it, and then call that class from wherever you need?

Thanks, @toniperic

If I was going to do that, I would need to have a command for completely creating the Category object, given the request information, because I need the contents of the $filename variable to persist to the database. Since commands can't return values, I'd have to abstract basically that whole controller method out into a command.

Would you recommend that?

Okay... I've done a bit of refactoring...

In my controller I now have:

/**
 * @param CategoryRequest $request
 *
 * @return RedirectResponse
 */
public function store(CategoryRequest $request)
{
    $this->dispatch(
        new CreateCategory(
            $request->get('name'),
            $request->get('description'),
            $request->file('thumbnail')
        )
    );

    return redirect(route('admin.category.index'));
}

And my command looks like this:

<?php namespace App\Commands;

use Symfony\Component\HttpFoundation\File\UploadedFile;

class CreateCategory extends Command {

    /**
     * @var string
     */
    private $name;

    /**
     * @var string
     */
    private $description;

    /**
     * @var UploadedFile
     */
    private $thumbnail;

    /**
     * @return void
     */
    public function __construct($name, $description, UploadedFile $thumbnail)
    {
        $this->name = $name;
        $this->description = $description;
        $this->thumbnail = $thumbnail;
    }

    /**
     * @return string
     */
    public function name()
    {
        return $this->name;
    }

    /**
     * @return null|string
     */
    public function description()
    {
        return empty($this->description) ? null : $this->description;
    }

    /**
     * @return UploadedFile
     */
    public function thumbnail()
    {
        return $this->thumbnail;
    }

}

And my handler looks like this:

<?php namespace App\Handlers\Commands;

use App\Commands\CreateCategory;
use App\Model\Category;

class CreateCategoryHandler {

    /**
     * @param  CreateCategory  $command
     *
     * @return void
     */
    public function handle(CreateCategory $command)
    {
        $filename = time() . '-' . str_slug($command->name()) . '.jpg';
        $command->thumbnail()->move(public_path() . '/uploads/images/categories', $filename);

        Category::create(
            [
                'name'        => $command->name(),
                'description' => $command->description(),
                'thumbnail'   => $filename
            ]
        );
    }

}

Is this better? What about using the Filesystem component in the handler?

The moving part

    public function upload($id)
    {
        $destination = public_path() . '/files';
        $imagename = Request::file('file')->getClientOriginalName();

        Request::file('file')->move( $destination, $imagename );

        $this->repository->storeImage($id, $imagename);

        return $this->respondOk();
    }

@xsmall I'm not sure what you're doing there, to be honest. The image is an embedded property of the Category class, so it seems a bit weird to separate that out?

True on the default one, not sure why you're setting it as null anyway?

@bashy I'm setting it to null because I want the database column to actually contain null. If you don't explicitly set it to null, then Eloquent persists an empty string to the database, which isn't what should be there. If no description has been provided, I'd consider null to be a more applicable value in the database.

@mdavis1982 yeah, that's better. Not sure why you import and use UploadedFile as dependency. Couldn't you just stick to Filesystem component?

@toniperic That's what I'm asking, I guess. My command has an instance of UploadedFile because that's what you get from the request object. I'm wondering if there's a better way of using the filesystem component here.