Test Driven Wallpaper Delete - Part 2
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)
73msLet'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)
78msOur 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)
76msHmmm, 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)
77msHuh.
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)
75msCleaning 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)
78msWe 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)
74msYes, 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)
102msWe 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)
78msThe 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)
83msNow 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.
