Fixing A Bug In POST Guided By Behat

In this video we are going to start by fixing a bug in the POST implementation. Currently when POST'ing in some new data, the file and associated metadata are uploaded, but the resulting entity has the wrong displayedFileName property, as it is being incorrectly set during object creation.

Let's fix that.

First, we need to update the Behat scenario:

# src/AppBundle/Features/file.feature

    Scenario: User can add a new File
      When I send a multipart "POST" request to "/accounts/a1/files" with form data:
        | name            | filePath        |
        | a new file name | Image/pk140.jpg |
      Then the response code should be 201
       And the response header "Content-Type" should be equal to "application/json; charset=utf-8"
       And the I follow the link in the Location response header
       And the response should contain json:
        """
        {
          "originalFileName": "pk140.jpg",
          "guessedExtension": "jpg",
          "displayedFileName": "pk140.jpg",
          "fileSize": 8053
        }
        """

The displayedFileName property in the response needs to be updated:

       And the response should contain json:
        """
        {
          "originalFileName": "pk140.jpg",
          "guessedExtension": "jpg",
          "displayedFileName": "a new file name",
          "fileSize": 8053
        }
        """

There's no escaping human error sometimes :) Sooner or later, you will write out a test that passes, but the test itself is incorrect.

Now that we've updated the test, we should at this stage have a failing test. This is good. We have made a change to an assertion in our expected outcome, and the tests should tell us that our expectations are not being met.

Now, I covered the POST method in quite a lot of depth in the previous video, so be sure to watch that and read the write up if unsure as to what is happening overall.

Instead here I am going to continue on fixing this particular bug, as it gives a better indication of a real world problem, and something that would likely come up when working with this layout.

The FilesController::postAction doesn't really concern itself at all with the 'how' of file creation, it simply delegates that responsibility to the FileHandler. This narrows down the possible place(s) that may need to be changed to fix this problem.

// src/AppBundle/Handler/FileHandler.php

    /**
     * @param  array                 $parameters
     * @param  array                 $options
     * @return FileInterface
     */
    public function post(array $parameters, array $options = [])
    {
        $account = $this->getAccount();

        $options = array_replace_recursive([
            'validation_groups' => ['post'],
            'has_file'          => true,
        ], $options);

        $fileDTO = $this->formHandler->handle(
            new FileDTO(),
            $parameters,
            Request::METHOD_POST,
            $options
        ); /** @var $fileDTO FileDTO */

        $file = $this->factory->createFromUploadedFile($fileDTO->getUploadedFile());

        $fileContents = $this->uploadFilesystem->getFileContentsFromPath($fileDTO->getUploadedFile()->getFilePath());
        $this->filesystem->put($file->getInternalFileName(), $fileContents);

        $account->addFile($file);

        $this->repository->save($file);

        return $file;
    }

This looks more like it. In particular, we can see that a $file variable is being created from the uploaded file.

Whether a factory is needed here is a point for debate (hint: it isn't really needed), it is useful to at least see the implementation of this factory:

<?php

// src/AppBundle/Factory/FileFactory.php

namespace AppBundle\Factory;

use AppBundle\Entity\File;
use AppBundle\Model\UploadedFileInterface;
use Symfony\Component\HttpFoundation\File\UploadedFile;

/**
 * Class FileFactory
 * @package AppBundle\Factory
 */
class FileFactory implements FileFactoryInterface
{
    /**
     * @param string $originalFileName
     * @param string $internalFileName
     * @param string $guessedExtension
     * @param int $fileSize
     * @return File
     */
    public function create($originalFileName, $internalFileName, $guessedExtension, $fileSize)
    {
        return new File($originalFileName, $internalFileName, $guessedExtension, $fileSize);
    }

    /**
     * @param UploadedFileInterface $uploadedFile
     * @return File
     */
    public function createFromUploadedFile(UploadedFileInterface $uploadedFile)
    {
        $internalFileName = sha1(uniqid(mt_rand(), true));

        return $this->create(
            $uploadedFile->getOriginalFileName(),
            $internalFileName,
            $uploadedFile->getFileExtension(),
            $uploadedFile->getFileSize()
        );
    }
}

We have an ability to create a File entity from something that implements UploadedFileInterface. But the problem we have is that UploadedFileInterface doesn't concern itself with the display name... maybe it could, but in this implementation, it does not:

<?php

// src/AppBundle/Model/UploadedFileInterface.php

namespace AppBundle\Model;

/**
 * Interface UploadedFileInterface
 * @package AppBundle\Model
 */
interface UploadedFileInterface
{
    /**
     * @return string
     */
    public function getFilePath();

    /**
     * @return string
     */
    public function getOriginalFileName();

    /**
     * @return string
     */
    public function getFileExtension();

    /**
     * @return string
     */
    public function getMimeType();

    /**
     * @return int
     */
    public function getFileSize();
}

This presents us with a bit of a problem. We are passing the UploadedFile to the factory, but we are not passing in the display name.

We could update the createFromUploadedFile method to also pass in the display file name as a second, optional parameter. But I don't like that. Personal preference I guess.

Instead, I think the best place for this - currently - is to be inside the FileHandler::post method:

$file = $this->factory->createFromUploadedFile($fileDTO->getUploadedFile());
$file->changeDisplayedFileName($fileDTO->getName());

And remember, this is because the displayed file name property is something we submitted via the form (and therefore, on the File DTO), but it is not a property of the UploadedFile.

Is this perfect? Not really. It does the job though. And now we should have a passing test again.

As ever with a system that isn't trivial / created solely for demonstration, there are always design decisions to be made, and they aren't always delightfully elegant, unfortunately.