Finally, It Works!


As we saw a couple of videos ago, the problem we have is that when this form is submitted, there's a bunch of missing - but required - data that are needed to satisfy the requirements of our entity.

Looking back at our original prototype, we had already encountered and fixed this problem:

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

    public function prePersist(LifecycleEventArgs $eventArgs)
    {
        // * snip *

        //   - update the entity with additional info
        [
            0 => $width,
            1 => $height,
        ] = getimagesize($newFileLocation);

        $entity
            ->setFilename(
                $file->getClientOriginalName()
            )
            ->setHeight($height)
            ->setWidth($width)
        ;

The gist of this is that we are using getimagesize, passing in the path to the wallpaper image file.

All being well, this function will return an array of 7 data elements, most of which we don't care about.

Now, this returned array does not use named keys. By this I mean the data will look something like this:

$data = [
    0 => 'first bit of data',
    1 => 'second bit of data',
    2 => 'third bit of data',
];

If the data were nicely keyed, it might look something like this:

$data = [
    'first'  => 'first bit of data',
    'second' => 'second bit of data',
    'third'  => 'third bit of data',
];

Why this matters is that when we use the PHP 7 short hand destructuring technique to access our data:

        [
            0 => $width,
            1 => $height,
        ] = getimagesize($newFileLocation);

We are saying that the value in index 0 should become a variable named $width, and index 1 should become a variable named $height. If the keys were named in a human-friendly way, this code may be a touch more obvious.

Anyway, once we have this data, we can programmatically update our Wallpaper entity, saving ourselves (and other system users) the burden of having to manually check the width, height, and provide the filename.

This process works well in our prototype. All we are going to do here is to port this concept across to our tested approach.

Let's update the assertions we're making in our spec for WallpaperListener:

// /spec/AppBundle/Event/Listener/WallpaperListenerSpec.php

    function it_can_prePersist(
        EntityManagerInterface $em,
        SymfonyUploadedFile $symfonyUploadedFile
    )
    {
        /* snip */

        $this->prePersist($eventArgs)->shouldReturn(true);

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

        // our new stuff below:

        /**
        // $outcome = some way of getting the Wallpaper entity

        $outcome->getFilename()->shouldReturn($fakeFilename);
        $outcome->getWidth()->shouldReturn(1024);
        $outcome->getHeight()->shouldReturn(768);
        */
    }

We're going to need to fake the width (1024) and height (768), but we will get to that shortly.

Our current primary test assertion is that, should everything go to plan, prePersist will return true.

As part of our testing, we would like to validate that the Wallpaper entity has a number of expected properties.

Therefore, I'm going to switch from returning true, to returning the Wallpaper entity object.

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

    public function prePersist(LifecycleEventArgs $eventArgs)
    {
        // * snip *

        // - return true;

        return $entity;
    }

After making this change, we should be able to more easily validate our assumptions.

As soon as this change is made, our tests will start failing:

Those darned pesky tests, always breaking when we change the behaviour of our system! :)

php vendor/bin/phpspec run

AppBundle/Event/Listener/WallpaperUploadListener
  48  - it can prePersist
      expected true, but got [obj:AppBundle\Entity\Wallpaper].

                                   91%                                     8%    12
4 specs
12 examples (11 passed, 1 failed)
95ms

We can change the test here:

// /spec/AppBundle/Event/Listener/WallpaperListenerSpec.php

    function it_can_prePersist(
        EntityManagerInterface $em,
        SymfonyUploadedFile $symfonyUploadedFile
    )
    {
        /* snip */

        // this is the changed line
        $this
            ->prePersist($eventArgs)
            ->shouldReturnAnInstanceOf(Wallpaper::class)
        ;

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

        /**
        // $outcome = some way of getting the Wallpaper entity

        $outcome->getFilename()->shouldReturn($fakeFilename);
        $outcome->getWidth()->shouldReturn(1024);
        $outcome->getHeight()->shouldReturn(768);
        */
    }

And this test now checks that the returned value from prePersist is an instance of our Wallpaper entity. That's quite useful.

We could also store the outcome of $this->prePersist into $outcome, and then do further assertions against the returned object:

        // this is the changed line
        $this
            ->prePersist($eventArgs)
            ->shouldReturnAnInstanceOf(Wallpaper::class)
        ;

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

        $outcome = $this->prePersist($eventArgs);

        $outcome->getFilename()->shouldReturn($fakeFilename);
        $outcome->getWidth()->shouldReturn(1024);
        $outcome->getHeight()->shouldReturn(768);
    }

Maybe we could tidy this up a touch though:

        $outcome = $this->prePersist($eventArgs);

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

        $outcome->shouldBeAnInstanceOf(Wallpaper::class)
        $outcome->getFilename()->shouldReturn($fakeFilename);
        $outcome->getWidth()->shouldReturn(1024);
        $outcome->getHeight()->shouldReturn(768);
    }

This feels a little cleaner, and clearer to me. Feel free to disagree though. Notice the change in language from shouldReturnAnInstanceOf to shouldBeAnInstanceOf. It's subtle, but it's better for my tastes.

Is It Passing?

No, of course it's not. That would be too easy.

php vendor/bin/phpspec run

AppBundle/Event/Listener/WallpaperUploadListener
  48  - it can prePersist
      warning: getimagesize(/some/new/fake/some.file): failed to open stream: No such file or directory in
      /path/to/my/wallpaper/src/AppBundle/Event/Listener/WallpaperUploadListener.php line 55

                                   91%                                     8%    12
4 specs
12 examples (11 passed, 1 broken)
35ms

We've run afoul of a very similar problem to that which kicked this whole process off:

We're using a PHP function (getimagesize) and that is taking our given file path at face value. In other words, PHP is blissfully unaware that the fake path we are using is purely for test purposes.

What's the solution here then?

Control, control, you must learn control!

Yoda is the little green dude that just keeps on giving.

What we're doing here is exposing the guts of our implementation.

Our test logic is quite loose. We aren't checking the absolute specifics, but rather that the eventual outcomes are as expected.

In contrast, our code itself is extremely specific. PhpSpec is highlighting a bad design decision. We could refactor our code here to delegate the 'how' part of getting an image's dimensions to a service. We can then easily mock this service as part of this test.

Let's get PhpSpec to do most of the dirty work for us:

php vendor/bin/phpspec desc AppBundle/Service/ImageFileDimensionsHelper

Specification for AppBundle\Service\ImageFileDimensionsHelper created in /path/to/my/wallpaper/spec/AppBundle/Service/ImageFileDimensionsHelperSpec.php.

And then run this new test file:

php vendor/bin/phpspec run spec/AppBundle/Service/ImageFileDimensionsHelperSpec.php

AppBundle/Service/ImageFileDimensionsHelper
  11  - it is initializable
      class AppBundle\Service\ImageFileDimensionsHelper does not exist.

                                      100%                                       1
1 specs
1 example (1 broken)
18ms

  Do you want me to create `AppBundle\Service\ImageFileDimensionsHelper` for
  you?
                                                                         [Y/n]
y
Class AppBundle\Service\ImageFileDimensionsHelper created in /path/to/my/wallpaper/src/AppBundle/Service/ImageFileDimensionsHelper.php.

                                      100%                                       1
1 specs
1 example (1 passed)
11ms

Easy enough.

This does raise a question or two though:

  • What should we test in this ImageFileDimensionsHelper class?
  • What should this class return?

The first question is a bit of a grey area for me. From my point of view there's very little we can test here. We know PhpSpec will have a minor meltdown if we try to test the actual getimagesize function. We could set up a fake image that we know is always going to be in our project, but is this worthwhile?

Potentially controversially, I'm not going to do anything further with regards to testing this class. It's isolated enough for me. Your opinion may differ, and I'd love to hear contrary view points. I'm always open to reviewing my own coding practices.

The second question is a bit easier to answer. Do we return an array mimicking the return value of getimagesize, should we go for two methods - getImageWidth, getImageHeight - or something else?

I'm going to go with two methods.

My 'why' for this is because, for me, it's simply a nicer developer experience:

<?php

// src/AppBundle/Service/ImageFileDimensionsHelper.php

namespace AppBundle\Service;

/**
 * Class ImageFileDimensionsHelper
 * @package AppBundle\Service
 */
class ImageFileDimensionsHelper
{
    /**
     * @var array $imageSizeAttributes
     */
    private $imageSizeAttributes;

    /**
     * @param string $filepath
     */
    public function setImageFilePath(string $filepath)
    {
        $this->imageSizeAttributes = getimagesize($filepath);
    }

    /**
     * @return int
     */
    public function getWidth()
    {
        try {
            return (int) $this->imageSizeAttributes[0];
        } catch (\Exception $e) {
            return 0;
        }
    }

    /**
     * @return int
     */
    public function getHeight()
    {
        try {
            return (int) $this->imageSizeAttributes[1];
        } catch (\Exception $e) {
            return 0;
        }
    }
}

Honestly thats about the very limit of the amount of code I want to write in a class that's not tested. It's small enough that I can quite easily "run it" in my head and understand it, but there's enough stuff happening here that I might miss something and inadvertently introduce a bug.

With all these changes, we need to make sure we have all the Symfony service definitions in place so that when we try this in the browser, our WallpaperUploadListener is actually triggered:

# app/config/services.yml

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

    app.service.image_file_dimensions_helper:
        class: AppBundle\Service\ImageFileDimensionsHelper

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

    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"
            - "@app.service.image_file_dimensions_helper"
        tags:
            - { name: doctrine.event_listener, event: prePersist }

From the service definition for the WallpaperUploadListener we can see that we are now injecting all three of our created services.

We need to update the WallpaperUploadListenerSpec accordingly:

use AppBundle\Service\ImageFileDimensionsHelper;
// others removed for brevity

class WallpaperUploadListenerSpec extends ObjectBehavior
{
    private $fileMover;
    private $wallpaperFilePathHelper;
    private $imageFileDimensionsHelper;

    function let(
        FileMover $fileMover,
        WallpaperFilePathHelper $wallpaperFilePathHelper,
        ImageFileDimensionsHelper $imageFileDimensionsHelper
    )
    {
        $this->beConstructedWith($fileMover, $wallpaperFilePathHelper, $imageFileDimensionsHelper);

        $this->fileMover = $fileMover;
        $this->wallpaperFilePathHelper = $wallpaperFilePathHelper;
        $this->imageFileDimensionsHelper = $imageFileDimensionsHelper;
    }

    // * snip *

    function it_can_prePersist(
        LifecycleEventArgs $eventArgs,
        FileInterface $file
    )
    {
        $fakeTempPath = '/tmp/some.file';
        $fakeFilename = 'some.file';

        $file->getPathname()->willReturn($fakeTempPath);
        $file->getFilename()->willReturn($fakeFilename);

        $wallpaper = new Wallpaper();
        $wallpaper->setFile($file->getWrappedObject());

        $eventArgs->getEntity()->willReturn($wallpaper);

        $fakeNewFilePath = '/some/new/fake/' . $fakeFilename;
        $this->wallpaperFilePathHelper->getNewFilePath($fakeFilename)->willReturn($fakeNewFilePath);

        // all the new stuff
        $this->imageFileDimensionsHelper->setImageFilePath($fakeNewFilePath)->shouldBeCalled();
        $this->imageFileDimensionsHelper->getWidth()->willReturn(1024);
        $this->imageFileDimensionsHelper->getHeight()->willReturn(768);

        $outcome = $this->prePersist($eventArgs);

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

        $outcome->shouldBeAnInstanceOf(Wallpaper::class);
        $outcome->getFilename()->shouldReturn($fakeFilename);
        $outcome->getWidth()->shouldReturn(1024);
        $outcome->getHeight()->shouldReturn(768);
    }

When we run the tests now, we should expect a fail. After all, we have just made some changes.

php vendor/bin/phpspec run

AppBundle/Event/Listener/WallpaperUploadListener
  55  - it can prePersist
      method `Double\FileInterface\P11::getClientOriginalName()` not found.

It looks like we have some loose ends to tie up.

Let's make the required changes:

<?php

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

namespace AppBundle\Event\Listener;

use AppBundle\Service\ImageFileDimensionsHelper;
// other stuff removed

class WallpaperUploadListener
{
    /**
     * @var FileMover
     */
    private $fileMover;
    /**
     * @var WallpaperFilePathHelper
     */
    private $wallpaperFilePathHelper;
    /**
     * @var ImageFileDimensionsHelper
     */
    private $imageFileDimensionsHelper;

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

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

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

        /**
         * @var $entity Wallpaper
         */

        $file = $entity->getFile();

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

        $this->fileMover->move(
            $file->getPathname(),
            $newFilePath
        );

/**
// the old approach
        $entity
            ->setFilename(
                $file->getClientOriginalName()
            )
            ->setHeight()
            ->setWidth($width)
        ;

        return $entity;
*/

        // new approach
        $this->imageFileDimensionsHelper->setImageFilePath($newFilePath);

        //   - update the entity with additional info
        return $entity
            ->setFilename(
                $file->getFilename()
            )
            ->setHeight(
                $this->imageFileDimensionsHelper->getHeight()
            )
            ->setWidth(
                $this->imageFileDimensionsHelper->getWidth()
            )
        ;
    }

    public function preUpdate(PreUpdateEventArgs $eventArgs)
    {
    }
}

And with that we are back to a set of passing tests:

 php vendor/bin/phpspec run
                                      100%                                       13
5 specs
13 examples (13 passed)
82ms

All that's left to do at this stage is test everything works in the browser (which it should!).

There is a visual issue that we still need to address.

I'm intrigued now - what do you think of the outcome here so far? Is it better or worse than when we did all this without tests? Has the system design really improved sufficiently to justify all the extra effort?

Code For This Course

Get the code for this course.

Episodes

# Title Duration
1 Introduction and Site Demo 02:14
2 Setup and a Basic Wallpaper Gallery 08:43
3 Pagination 08:24
4 Adding a Detail View 04:47
5 Creating a Home Page 11:14
6 Creating our Wallpaper Entity 07:50
7 Wallpaper Setup Command - Part 1 - Symfony Commands As a Service 05:57
8 Wallpaper Setup Command - Part 2 - Injection Is Easy 08:53
9 Wallpaper Setup Command - Part 3 - Doing It With Style 05:37
10 Doctrine Fixtures - Part 1 - Setup and Category Entity Creation 08:52
11 Doctrine Fixtures - Part 2 - Relating Wallpapers with Categories 05:56
12 EasyAdminBundle - Setup and Category Configuration 06:02
13 EasyAdminBundle - Wallpaper Setup and List View 07:46
14 EasyAdminBundle - Starting with Wallpaper Uploads 05:57
15 Testing with PhpSpec to Guide Our Implementation 03:39
16 Using PhpSpec to Test our FileMover 05:34
17 Symfony Dependency Testing with PhpSpec 08:47
18 Defensive Counter Measures 06:33
19 No Tests - Part 1 - Uploading Files in EasyAdminBundle 11:01
20 No Tests - Part 2 - Uploading Files in EasyAdminBundle 07:05
21 Don't Mock What You Don't Own 09:36
22 You've Got To Take The Power Back 07:36
23 Making Symfony Work For Us 08:56
24 Testing The Wallpaper File Path Helper 15:11
25 Finally, It Works! 14:56
26 Why I Prefer Not To Use Twig 16:51
27 Fixing The Fixtures 11:20
28 Untested Updates 14:30
29 Untested Updates Part Two - Now We Can Actually Update 06:33
30 Adding an Image Preview On Edit 12:31
31 Delete Should Remove The Wallpaper Image File 11:02
32 Getting Started Testing Wallpaper Updates 10:02
33 Doing The Little Before The Big 08:13
34 Tested Image Preview... Sort Of :) 07:36
35 Finishing Up With a Tested Wallpaper Update 10:41
36 Test Driven Wallpaper Delete - Part 1 11:06
37 Test Driven Wallpaper Delete - Part 2 11:57
38 EasyAdminBundle Login Form Tutorial 08:01