No Tests - Part 1 - Uploading Files in EasyAdminBundle

We've now defended against 'accidentally' running our listener code for every entity.

We know now that this prePersist method will only go further if the given entity is an instance of Wallpaper.

Now we do need to figure out how move will be called with the expected arguments.

Let's quickly recap our original test:

// /spec/AppBundle/Event/Listener/WallpaperUploadListenerSpec.php

    function it_can_prePersist(LifecycleEventArgs $eventArgs)
    {
        $fakeTempPath = '/tmp/some.file';
        $fakeRealPath = '/path/to/my/project/some.file';

        $this->prePersist($eventArgs);

        $this->fileMover->move($fakeTempPath, $fakeRealPath)->shouldHaveBeenCalled();
    }

We need to know two things:

• How to get the temporary path
• How to get the path we wish to use in the longer term

What we know is:

• The temporary path will be provided to us by PHP
• Our real path will be $projectRoot . '/web/images/' . $fileName

Now this is, in my experience, where things get super interesting.

What we are about to do is to cover two different approaches to this same problem.

First, we are going to do this without tests.

Have you ever thought to yourself:

I will just write this code now, and then I promise (fingers-crossed) that I will come back and write tests for this later.

I know I have.

Skipping the tests immediately is an easy productivity win.

Unfortunately, the curve against this practice remaining favourable over time is a drop off a cliff.

Still, we do this to ourselves.

I've often wondered what the outcome of doing a task with AND without tests would be. The same task.

If we skip the tests, what impact will this actually have on our code?

What about on the overall system design?

We are going to build this next step without tests.

Then, we are going to switch branches back to our starting point, and re-write the exact same solution, but test-driven.

The questions I'm posing to answer is:

Will these two approaches lead to the same, or different overall system design?

No tests. Shield's Up, Red Alert!

It's best if you imagine a picture of William Riker for the above sub-title.

On a smaller system, which this objectively is, it can be very easy to retain the entire system work flow in your head.

If all you ever build is small systems - ones you use exclusively - then having no tests, in my opinion, is fine.

The only person you will ever impact is yourself. Assuming it's a small system, one for hobby work or learning, then tests will slow you down, and are another new thing to have to learn on top of the mountain of "stuff" you already feel you need to know to feel comfortable using Symfony.

Deep breath. And relax.

You can skip all of what I am about to show you.

I did for ages.

But then one day you will want to become a better developer, and take the greatest pride in your work.

When I deliver code, I want it to work, first time. I'm guessing you do, too.

It's good for your career, but also, fixing bugs sucks. If there was a single way to write code in a manner that consistently produced code with fewer bugs, wouldn't it make sense to do this?

I hope you're in agreement, but for the benefit of science, let's start with the alternative.

Prototyping

We start with our listener.

<?php

// src/AppBundle/Event/Listener/WallpaperUploadListener.php

namespace AppBundle\Event\Listener;

use Symfony\Component\HttpFoundation\File\UploadedFile;
use Doctrine\ORM\Event\LifecycleEventArgs;
use Doctrine\ORM\Event\PreUpdateEventArgs;
use AppBundle\Entity\Wallpaper;
use AppBundle\Service\FileMover;

class WallpaperUploadListener
{
    private $fileMover;

    public function __construct(FileMover $fileMover)
    {
        $this->fileMover = $fileMover;
    }

    public function prePersist(LifecycleEventArgs $args)
    {
        if (false === $eventArgs->getEntity() instanceof Wallpaper) {
            return false;
        }

        // todo:
        // - move the uploaded file
        // - update the Wallpaper entity with new info

        return true;
    }
}

We need to move the uploaded file from the temporary, to its longer term location.

At this stage, this is simply a path on our local hard disk. There are so many improvements that we could make on this.

Earlier, I said we need to know two things:

• How to get the temporary path
• How to get the path we wish to use in the longer term

We can look at the methods that Symfony's UploadedFile class will give us, and work from there.

To recap, Symfony will turn the data submitted via the form into a Wallpaper object for us.

One of the properties we have on our Wallpaper is file. This is being used to store the object representing the uploaded file from the form. When using the File form field type, the uploaded file will be represented by an instance of Symfony's UploadedFile.

By accessing that object we can figure out properties of the file.

From these properties we can figure out both the file's current, temporary location, and also determine the path we want to use in the longer term.

Figuring out the temporary path of an UploadedFile is deceptively easy.

UploadedFile extends File which extends \SplFileInfo.

There is a method on \SplFileInfo that gives us exactly what we want:

getPathName

And we also need to figure out the new file name.

Now, looking at the bigger picture here, we are running a Wallpaper Website. We're going to have a bunch of wallpaper images for people to browse, and we want to maximise our chances of people finding us via Google, etc.

Therefore, if we are spending time diligently crafting beautiful file names for SEO Reasons, then the last thing we want is to have our filenames mangled on upload.

Fortunately we can get access to the uploaded file's name by using getClientOriginalName().

<?php

// src/AppBundle/Event/Listener/WallpaperUploadListener.php

namespace AppBundle\Event\Listener;

use Symfony\Component\HttpFoundation\File\UploadedFile;
use Doctrine\ORM\Event\LifecycleEventArgs;
use Doctrine\ORM\Event\PreUpdateEventArgs;
use AppBundle\Entity\Wallpaper;
use AppBundle\Service\FileMover;

class WallpaperUploadListener
{
    private $fileMover;

    public function __construct(FileMover $fileMover)
    {
        $this->fileMover = $fileMover;
    }

    public function prePersist(LifecycleEventArgs $args)
    {
        $entity = $eventArgs->getEntity();

        if (false === $entity instanceof Wallpaper) {
            return false;
        }

        /**
         * @var $entity Wallpaper
         */

        // get access to the file
        $file = $entity->getFile();

        // todo:
        //  - figure out new file path
        $newFilePath = '/this/is/nonsense/' . $file->getClientOriginalName();

        // move the uploaded file
        // args: $currentPath, $newPath
        $this->fileMover->move(
            $file->getPathname(),
            $newFilePath
        );

        // todo:
        // - update the Wallpaper entity with new info

        return true;
    }
}

This method feels like it's doing too much to me already.

It's tempting to inject the path the project's kernel.root_dir here (or kernel.project_dir in Symfony 3.3+), and replace the this/is/nonsense part of the variable.

The seemingly better way to do this would be to create a new, more specialised implementation of the file mover - a LocalFilesystemFileMover.

Quite a crazy specific name, but it does what it says on the tin.

I'm working on the assumption that we will only ever want to upload Wallpaper files. This is a wallpaper website after all. If we needed to upload other types of file then maybe we might even consider calling this a WallpaperLocalFilesystemFileMover, or similar.

There's a fine line with this sort of thing, and it's fairly easy to go overly abstract which only serves to cloud more than clarify.

Continuing on, let's extract an Interface from our FileMover:

<?php

// src/AppBundle/Service/FileMover.php

interface FileMover
{
    public function move($existingFilePath, $newFilePath);
}

It doesn't matter specifically where the existing file path, or the new file paths are. They may be on local disk, or on S3, or Dropbox, or FTP... hopefully this interface is sufficient to cover all implementations. We shall see.

Recently I've been trying to stop using the Interface suffix on my interfaces:

FileMover vs FileMoverInterface.

I currently prefer the former. There is no right or wrong, to the very best of my knowledge.

We already have our FileMover.php file, which I'm now renaming to LocalFilesystemFileMover.php:

<?php

// src/AppBundle/Service/LocalFilesystemFileMover.php

namespace AppBundle\Service;

use Symfony\Component\Filesystem\Filesystem;

class LocalFilesystemFileMover implements FileMover
{
    // rest of the implementation remains the same

As we've changed the name of this implementation, we will need to update the corresponding service definition as well:

# app/config/services.yml

services:

    app.service.local_filesystem_file_mover:
        class: AppBundle\Service\LocalFilesystemFileMover
        arguments:
            - "@filesystem"

As we've changed the name of this service, we will also need to update any other service definitions that use this service:

# app/config/services.yml

    app.doctrine_event_listener.wallpaper_upload_listener:
        class: AppBundle\Event\Listener\WallpaperUploadListener
        arguments:
            # - '@app.service.file_mover'
            - '@app.service.local_filesystem_file_mover'

Even though we have a more specialised FileMover implementation, so long as this class implements FileMover, we can inject it without issue. Think of this FileMover interface as our contract. So long as the class we pass in meets the contract, it can be used - or swapped out for a different class that also fulfills that contract.

Extracting out to the FileMover interface allows us to decouple the specifics of the way files are named, and stored, from the process of "uploading a wallpaper".

By relying on an interface for our FileMover::move call, we could - in theory - more easily swap out saving files on our servers local hard disk, to a more robust storage mechanism such as Amazon S3.

One way in which we might do this later on is by making use of FlySystem. This would make it super easy to store on local disk, or Amazon S3, or Dropbox. Right now, however, adding this in seems like overkill.

We now have the FileMover interface, and a concrete LocalFilesystemFileMover implementation of that interface.

Into the LocalFilesystemFileMover we might think to inject the kernel.root_dir path, and hide away a bunch of unnecessary implementation details from the wider WallpaperUploadListener.

However, if we look at the signature for move, we expect to be given a full path to move too: $newFilePath.

It's not really the job of the FileMover implementation to figure out the $newFilePath.

Even though it seems overkill to do so, I'm going to create a separate service that can help us determine the $newFilePath. It's a very simple service with nothing new that we haven't already seen before:

<?php

// src/AppBundle/Service/WallpaperFilePathHelper.php

namespace AppBundle\Service;

class WallpaperFilePathHelper
{
    /**
     * @var string
     */
    private $wallpaperFileDirectory;

    public function __construct(string $wallpaperFileDirectory)
    {
        $this->wallpaperFileDirectory = $wallpaperFileDirectory;
    }

    /**
     * @param string $newFileName
     * @return string
     */
    public function getNewFilePath(string $newFileName)
    {
        return $this->wallpaperFileDirectory . $newFileName;
    }
}

Don't forget the service definition:

# app/config/services.yml

services:

    app.service.wallpaper_file_path_helper:
        class: AppBundle\Service\WallpaperFilePathHelper
        arguments:
            - "%kernel.root_dir%/../web/images/"

Now, let's also inject the WallpaperFilePathHelper into our listener, and get a new file path which we can pass on to the LocalFilesystemFileMover::move call:

<?php

// src/AppBundle/Event/Listener/WallpaperUploadListener.php

namespace AppBundle\Event\Listener;

use Symfony\Component\HttpFoundation\File\UploadedFile;
use Doctrine\ORM\Event\LifecycleEventArgs;
use Doctrine\ORM\Event\PreUpdateEventArgs;
use AppBundle\Entity\Wallpaper;
use AppBundle\Service\FileMover;
use AppBundle\Service\WallpaperFilePathHelper;

class WallpaperUploadListener
{
    private $fileMover;
    private $wallpaperFilePathHelper;

    public function __construct(
        FileMover $fileMover,
        WallpaperFilePathHelper $wallpaperFilePathHelper
    )
    {
        $this->fileMover = $fileMover;
        $this->wallpaperFilePathHelper = $wallpaperFilePathHelper;
    }

    public function prePersist(LifecycleEventArgs $eventArgs)
    {
        $entity = $eventArgs->getEntity();

        if (false === $entity instanceof Wallpaper) {
            return false;
        }

        /**
         * @var $entity Wallpaper
         */

        // get access to the file
        $file = $entity->getFile();

        $newFilePath = $this->wallpaperFilePathHelper
            ->getNewFilePath(
                $file->getClientOriginalName()
            )
        ;

        // move the uploaded file
        // args: $currentPath, $newPath
        $this->fileMover->move(
            $file->getPathname(),
            $newFilePath
        );

        // todo:
        // - update the Wallpaper entity with new info

        return true;
    }
}

Again, let's update the service definition to correctly inject this additional dependency:

# app/config/services.yml

    app.doctrine_event_listener.wallpaper_upload_listener:
        class: AppBundle\Event\Listener\WallpaperUploadListener
        arguments:
            - '@app.service.local_filesystem_file_mover'
            - "@app.service.wallpaper_file_path_helper"

Remember, the order of these arguments matters.

We're about half way there.

We still need to hook up our listener so that the prePersist (and preUpdate) methods will be fired when appropriate.

We also need to tease out some information from the uploaded wallpaper file to help populate our entity - such as the width and height of the image, the slug, and make sure we're storing the correct information about the file on the entity itself.

We will get onto this in the very next video.