Towards the end of the previous video we were about half way towards a tested Wallpaper Delete implementation. In this video we are continuing on with this process, aiming to be able to reliably delete uploaded Wallpaper entities and associated image files.

With the LocalFilesystemFileDeleterSpec passing, we can return our attention to the WallpaperListener, which needs some service passing in that is capable of deleting files, in order to get us to a passing state.

php vendor/bin/phpspec run spec/AppBundle/Event/Listener/WallpaperListenerSpec.php

AppBundle/Event/Listener/WallpaperListener
 143  - it can preRemove
      property fileDeleter not found.

AppBundle/Event/Listener/WallpaperListener
 155  - it will not continue with preRemove if not a Wallpaper instance
      todo: write pending example

    14%                               71%                               14%      7
1 specs
7 examples (5 passed, 1 pending, 1 broken)
73ms

Let's start by updating the let function inside our WallpaperListenerSpec to receive a mock of our FileDeleter interface:

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

    private $fileMover;
    private $wallpaperFilePathHelper;
    private $imageFileDimensionsHelper;
    private $fileDeleter;

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

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

This gets us a little further:

php vendor/bin/phpspec run spec/AppBundle/Event/Listener/WallpaperListenerSpec.php

AppBundle/Event/Listener/WallpaperListener
 147  - it can preRemove
      no calls have been made that match:
        Double\FileDeleter\P30->delete(exact("fake-filename.jpg"))
      but expected at least one.

AppBundle/Event/Listener/WallpaperListener
 159  - it will not continue with preRemove if not a Wallpaper instance
      todo: write pending example

    14%                               71%                               14%      7
1 specs
7 examples (5 passed, 1 pending, 1 failed)
78ms

Our next step is to update the real implementation of our WallpaperListener.

We need to update the __construct method to accept this new argument, and we will need to set this as a private property of our class:

<?php

// src/AppBundle/Event/Listener/WallpaperListener.php

namespace AppBundle\Event\Listener;

# new entry, others removed for brevity
use AppBundle\Service\FileDeleter;

class WallpaperListener
{
    /**
     * @var FileMover
     */
    private $fileMover;
    /**
     * @var WallpaperFilePathHelper
     */
    private $wallpaperFilePathHelper;
    /**
     * @var ImageFileDimensionsHelper
     */
    private $imageFileDimensionsHelper;
    /**
     * @var FileDeleter
     */
    private $fileDeleter;

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

Don't forget to update the service definition:

# app/config/services.yml

    app.event.listener.wallpaper_listener:
        class: AppBundle\Event\Listener\WallpaperListener
        arguments:
            - "@app.service.file_mover"
            - "@app.service.wallpaper_file_path_helper"
            - "@app.service.image_file_dimensions_helper"
            - "@app.service.local_filesystem_file_deleter"
        tags:
            - { name: doctrine.event_listener, event: prePersist }
            - { name: doctrine.event_listener, event: preUpdate }
            - { name: doctrine.event_listener, event: preRemove }

It's easily to forget, and if you're that way inclined, Symfony 3.3 onwards is moving towards a more automated approach to service configuration declaration.

We know the implementation we need thanks to our prototype. So let's add that in too:

    public function preRemove(LifecycleEventArgs $args)
    {
        /**
         * @var $entity Wallpaper
         */
        $entity = $args->getEntity();

        $this->fileDeleter->delete(
            $entity->getFilename()
        );

        $entity->setFile(null);
    }

This should probably pass now, right?

After all, this is code that we saw was working in our untested implementation.

php vendor/bin/phpspec run spec/AppBundle/Event/Listener/WallpaperListenerSpec.php

AppBundle/Event/Listener/WallpaperListener
 147  - it can preRemove
      exception [err:TypeError("Argument 1 passed to Double\AppBundle\Entity\Wallpaper\P31::setFile() must implement interface AppBundle\Model\FileInterface, null given, called in /path/to/my/wallpaper/src/AppBundle/Event/Listener/WallpaperListener.php on line 110")] has been thrown.

AppBundle/Event/Listener/WallpaperListener
 159  - it will not continue with preRemove if not a Wallpaper instance
      todo: write pending example

    14%                               71%                               14%      7
1 specs
7 examples (5 passed, 1 pending, 1 broken)
76ms

Hmmm, not quite.

Let's look at that error message a little more closely:

"Argument 1 passed to Double\AppBundle\Entity\Wallpaper\P31::setFile() must implement interface AppBundle\Model\FileInterface, null given,

And if we look at Wallpaper::setFile:

    /**
     * @param FileInterface $file
     * @return Wallpaper
     */
    public function setFile(FileInterface $file)
    {
        $this->file = $file;

        if ($file) {
            $this->setUpdatedAt();
        }

        return $this;
    }

Right, so setFile expects an instance of FileInterface, and won't accept anything else.

That's another potential bug we just caught.

Fixing this is easy enough, we just need to accept either a FileInterface, or null:

    /**
     * @param FileInterface $file
     * @return Wallpaper
     */
    public function setFile(FileInterface $file = null)
    {

Somewhat strange notation, but it achieves our desired goal.

And we are surely now passing?

php vendor/bin/phpspec run spec/AppBundle/Event/Listener/WallpaperListenerSpec.php

AppBundle/Event/Listener/WallpaperListener
 147  - it can preRemove
      method call:
        - setFile(null)
      on Double\AppBundle\Entity\Wallpaper\P31 was not expected, expected calls were:
        - getFilename()

AppBundle/Event/Listener/WallpaperListener
 159  - it will not continue with preRemove if not a Wallpaper instance
      todo: write pending example

    14%                               71%                               14%      7
1 specs
7 examples (5 passed, 1 pending, 1 broken)
77ms

Huh.

But didn't we set this up in our test code in the previous video?

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

    function it_can_preRemove(LifecycleEventArgs $eventArgs, Wallpaper $wallpaper)
    {
        $wallpaper->getFilename()->willReturn('fake-filename.jpg');
        $eventArgs->getEntity()->willReturn($wallpaper);

        $this->preRemove($eventArgs);

        $this->fileDeleter->delete('fake-filename.jpg')->shouldHaveBeenCalled();

        // this won't work as expected
        $wallpaper->setFile(null)->shouldHaveBeenCalled();
    }

There's a potential point of confusion here.

We can either mock, or spy. But not both at the same time.

Here we used Wallpaper as a mock:

$wallpaper->getFilename()->willReturn('fake-filename.jpg');

and then again, as a spy:

$wallpaper->setFile(null)->shouldHaveBeenCalled();

We need to use one approach only.

We will change up to be a mock:

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

    function it_can_preRemove(LifecycleEventArgs $eventArgs, Wallpaper $wallpaper)
    {
        $wallpaper->setFile(null)->shouldBeCalled();

        $wallpaper->getFilename()->willReturn('fake-filename.jpg');
        $eventArgs->getEntity()->willReturn($wallpaper);

        $this->preRemove($eventArgs);

        $this->fileDeleter->delete('fake-filename.jpg')->shouldHaveBeenCalled();
    }

Notice the tense of the method name change:

shouldHaveBeenCalled to shouldBeCalled

With this change we do now pass:

php vendor/bin/phpspec run spec/AppBundle/Event/Listener/WallpaperListenerSpec.php

AppBundle/Event/Listener/WallpaperListener
 159  - it will not continue with preRemove if not a Wallpaper instance
      todo: write pending example

    14%                                     85%                                  7
1 specs
7 examples (6 passed, 1 pending)
75ms

Cleaning Up

Even though we've done quite a lot in-between, by adding in the extra test:

"it will not continue with preRemove if not a Wallpaper instance"

We know we aren't quite done just yet. Nice, PhpSpec helps us out once again. Not so easy to forget important stuff :)

All we need here is to check a guard statement.

That said, we don't yet have that guard statement, nor a failing test.

Let's start with the test:

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

    function it_will_not_continue_with_preRemove_if_not_a_Wallpaper_instance(
        LifecycleEventArgs $eventArgs
    )
    {
        $eventArgs->getEntity()->willReturn(new Category());

        $this->preRemove($eventArgs)->shouldReturn(false);

        $this->fileDeleter->delete(
            Argument::any()
        )->shouldNotHaveBeenCalled();
    }

This test is almost identical to the tests we used to check our guard logic in preUpdate and prePersist scenarios.

Rather than receiving the happy path Wallpaper, here we instead receive a Category.

If we get anything other than a Wallpaper we don't want to continue.

Also we can double check that a delete attempt wasn't made.

Our test expectedly fails at this point:

php vendor/bin/phpspec run spec/AppBundle/Event/Listener/WallpaperListenerSpec.php

AppBundle/Event/Listener/WallpaperListener
 160  - it will not continue with preRemove if not a Wallpaper instance
      exception [err:Error("Call to undefined method AppBundle\Entity\Category::getFilename()")] has been thrown.

                                 85%                                     14%     7
1 specs
7 examples (6 passed, 1 broken)
78ms

We have no guard logic. Yikes.

We know how to fix this:

    public function preRemove(LifecycleEventArgs $args)
    {
        /**
         * @var $entity Wallpaper
         */
        $entity = $args->getEntity();

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

        // * snip *
    }

And now:

php vendor/bin/phpspec run spec/AppBundle/Event/Listener/WallpaperListenerSpec.php
                                      100%                                       7
1 specs
7 examples (7 passed)
74ms

Yes, it's copy / paste from the upload method. It's duplication. I still don't feel like this is a good candidate for its own method. You may disagree here, and I'm not going to split hairs. Do what you feel is best.

At this point all our tests across the project should be passing:

php vendor/bin/phpspec run
                                      100%                                       31
9 specs
31 examples (31 passed)
102ms

We should be able to browse to our site and both create, and update existing Wallpapers. When we delete a wallpaper, the associated image file should be removed.

There is a bug here, however.

Yes, even with tests you will still get bugs, unfortunately.

If we create a new wallpaper, then edit the wallpaper, the existing wallpaper image file remains on disk.

Let's update our tests to cover this, and then write some code to stop it from happening.

First we will start with the prePersist test, as in this case we don't want to try and delete any existing files. This is a potential discussion point, as what happens if the persistence operation of a Wallpaper entity fails, but leaves on disk the image file we were trying to upload?

We would need to give that point a little further consideration.

In this case I am going to work down the happy path.

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

    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);

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

        $this->imageFileDimensionsHelper->setImageFilePath($fakeNewFileLocation)->shouldBeCalled();
        $this->imageFileDimensionsHelper->getWidth()->willReturn(1024);
        $this->imageFileDimensionsHelper->getHeight()->willReturn(768);

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

        /**
         * This is the new line
         */
        $this->fileDeleter->delete(Argument::any())->shouldNotHaveBeenCalled();

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

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

Fairly straightforward.

We already set up the $this->fileDeleter class property accessor as part of the preRemove test stuff we did previously.

Now all we want to assert here is that if we are in prePersist mode, we aren't trying to delete anything.

A little more involved is when we do need to first delete an existing file, in our preUpdate function:

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

    function it_can_preUpdate(
        PreUpdateEventArgs $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());
        /**
         * This is a new line
         */
        $wallpaper->setFilename($fakeFilename);

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

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

        $this->imageFileDimensionsHelper->setImageFilePath($fakeNewFileLocation)->shouldBeCalled();
        $this->imageFileDimensionsHelper->getWidth()->willReturn(1024);
        $this->imageFileDimensionsHelper->getHeight()->willReturn(768);

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

        /**
         * This is a new line
         */
        $this->fileDeleter->delete($fakeFilename)->shouldHaveBeenCalled();

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

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

When working with an existing Wallpaper we first would be best served deleting the existing image from disk before uploading the new one.

First we must set the filename, as we will want to delete any image file associated with that filename.

Secondly, we want to assert that the fileDeleter was called with the filename in this instance.

You might expect two failing tests here, but in fact we only get one fail. The other is already passing:

php vendor/bin/phpspec run spec/AppBundle/Event/Listener/WallpaperListenerSpec.php

AppBundle/Event/Listener/WallpaperListener
 112  - it can preUpdate
      no calls have been made that match:
        Double\FileDeleter\P24->delete(exact("some.file"))
      but expected at least one.

                                 85%                                     14%     7
1 specs
7 examples (6 passed, 1 failed)
78ms

The prePersist test is already passing because by default we aren't calling $this-fileDeleter at all, and that's essentially all that test is checking.

The preUpdate test does fail, which we were - hopefully - expecting.

We can employ the following code to fix this issue:

// src/AppBundle/Event/Listener/WallpaperListener.php

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

        /**
         * @var $entity Wallpaper
         */

        if ($entity->getFilename() !== null) {
            $this->fileDeleter->delete(
                $entity->getFilename()
            );
        }

        // * snip *
    }

If we have anything set on getFilename then try and delete the associated file.

Not a whole lot to think about, but it does the job.

php vendor/bin/phpspec run spec/AppBundle/Event/Listener/WallpaperListenerSpec.php
                                      100%                                       7
1 specs
7 examples (7 passed)
78ms

php vendor/bin/phpspec run
                                      100%                                       31
9 specs
31 examples (31 passed)
83ms

Now we can try to upload and update once again, and the original file should be gone from disk. Nice.

There are likely other bugs that remain. There's also a mess starting to take over our services file. We will get to that in time. For now, let's continue on and secure our back end from baddies.


Code For This Course

Get the code for this course.

Share This Episode

If you have found this video helpful, please consider sharing. I really appreciate it.


Episodes in this series

# 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:56
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:32
19 No Tests - Part 1 - Uploading Files in EasyAdminBundle 11:02
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:50
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:44
36 Test Driven Wallpaper Delete - Part 1 11:11
37 Test Driven Wallpaper Delete - Part 2 11:57
38 EasyAdminBundle Login Form Tutorial 08:01