Doing The Little Before The Big

Next on our task list is to implement the preUpdate event listener. As we have done this already in an untested manner, we will use that code and general procedure as the basis for our test driven approach.

One of the discoveries that we made in the untested approach was that the name WallpaperUploadListener was perhaps not that helpful. We shortened this to simply the WallpaperListener, and it would be good to update our tested implementation to match.

There are three files we need to change to make this possible:

1. Rename the files:

    renamed:    spec/AppBundle/Event/Listener/WallpaperUploadListenerSpec.php -> spec/AppBundle/Event/Listener/WallpaperListenerSpec.php
    renamed:    src/AppBundle/Event/Listener/WallpaperUploadListener.php -> src/AppBundle/Event/Listener/WallpaperListener.php

2. Open both of these files and do a find / replace for "Upload" with an empty string: "".

3. Update the service definition:

# app/config/services.yml

    app.event.listener.wallpaper_listener:
        class: AppBundle\Event\Listener\WallpaperListener

Now, re-run your test suite and hopefully you still have 18 passing tests.

Ok, whilst we have the services.yml file open, let's add in an entry for preUpdate:

# 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"
        tags:
            - { name: doctrine.event_listener, event: prePersist }
            - { name: doctrine.event_listener, event: preUpdate }

As we have covered already in the untested approach, we already have the vast majority of the logic we need to make the Update process work. Currently that logic is stored inside the prePersist method. We will need to extract that out.

We'll get to that in just a second.

One other thing we found out during our untested excursion was that the Wallpaper Upload process for the underlying file won't happen during an Update form submission unless at least one of the other properties of the Wallpaper object changes also. To very quickly recap why this is: it's because the File object is not a Doctrine entity and as such changes to it alone are not picked up by the entity manager.

We have a fix for this problem: timestamps.

During the untested approach we simply added the class properties (createdAt, updatedAt) and methods (setCreatedAt, getUpdatedAt, etc) to our Wallpaper entity.

In the real world, a nicer solution to this problem is to use Traits.

Traits allow you to define little snippets of code that you can use in other classes. This is really handy for commonly used bits of code that many unrelated classes need to use. Timestamps are a great example of this - many different types of entity benefit from having timestamps, and sharing a common base class just for this functionality would be very nasty.

As alluded too throughout this course, using a createdAt, and updatedAt are fairly useless bits of data if what you care about is what changed at that time. But again, that's a different discussion.

Traits have a bit of a bad reputation among the more vocal contingent of the PHP community, and in truth, I use them sparingly. Timestamping is a good use case. Here are some others.

<?php

// src/AppBundle/Entity/Traits/Timestampable.php

namespace AppBundle\Entity\Traits;

use Doctrine\ORM\Mapping as ORM;

trait Timestampable
{
    /**
     * @var \DateTime $createdAt Created at
     *
     * @ORM\Column(type="datetime", name="created_at")
     * @ORM\PrePersist()
     */
    protected $createdAt;

    /**
     * @var \DateTime $updatedAt Updated at
     *
     * @ORM\Column(type="datetime", name="updated_at")
     * @ORM\PreUpdate()
     */
    protected $updatedAt;

    /**
     * Get created at
     *
     * @return \DateTime Created at
     */
    public function getCreatedAt()
    {
        return $this->createdAt;
    }

    /**
     * Set created at
     *
     * @return $this
     */
    public function setCreatedAt()
    {
        $this->createdAt = new \DateTime('now');

        return $this;
    }

    /**
     * Get updated at
     *
     * @return \DateTime Updated at
     */
    public function getUpdatedAt()
    {
        return $this->updatedAt;
    }

    /**
     * Set updated at
     *
     * @return $this
     */
    public function setUpdatedAt()
    {
        $this->updatedAt = new \DateTime('now');

        return $this;
    }
}

Note here that I put the Timestampable trait inside the AppBundle\Entity\Traits namespace because this trait has Doctrine annotation logic added in, and the only place I would use this trait is within my own entities. Feel free to disagree / move around / change up to meet your own needs.

You may be wondering why I have Entity singular, but Traits plural. This is because Trait is a reserved keyword so will cause PHP to throw a Benny if used. As such, using the plural form is a simple, if ugly solution.

With the trait created, we also need to use it inside our Wallpaper entity:

<?php

namespace AppBundle\Entity;

use AppBundle\Model\FileInterface;
use Doctrine\ORM\Mapping as ORM;
use AppBundle\Entity\Traits

/**
 * Wallpaper
 *
 * @ORM\Table(name="wallpaper")
 * @ORM\Entity(repositoryClass="AppBundle\Repository\WallpaperRepository")
 * @ORM\HasLifecycleCallbacks() // <-- make sure to add this also
 */
class Wallpaper
{
    use Timestampable;

Don't forget to add in the @ORM\HasLifecycleCallbacks() annotation, it's easy to miss.

We've made a change to our entity here. In use'ing a trait, we have gained the two annotated properties from the trait, and as such must create a migration:

php bin/console doctrine:migrations:diff

Generated new migration class to "/path/to/my/wallpaper/app/DoctrineMigrations/Version20170729085036.php" from schema differences.
<?php

namespace Application\Migrations;

use Doctrine\DBAL\Migrations\AbstractMigration;
use Doctrine\DBAL\Schema\Schema;

/**
 * Auto-generated Migration: Please modify to your needs!
 */
class Version20170729085036 extends AbstractMigration
{
    /**
     * @param Schema $schema
     */
    public function up(Schema $schema)
    {
        // this up() migration is auto-generated, please modify it to your needs
        $this->abortIf($this->connection->getDatabasePlatform()->getName() !== 'mysql', 'Migration can only be executed safely on \'mysql\'.');

        $this->addSql('ALTER TABLE wallpaper ADD created_at DATETIME NOT NULL, ADD updated_at DATETIME NOT NULL');
    }

    /**
     * @param Schema $schema
     */
    public function down(Schema $schema)
    {
        // this down() migration is auto-generated, please modify it to your needs
        $this->abortIf($this->connection->getDatabasePlatform()->getName() !== 'mysql', 'Migration can only be executed safely on \'mysql\'.');

        $this->addSql('ALTER TABLE wallpaper DROP created_at, DROP updated_at');
    }
}

Looks good, let's apply it:

php bin/console doctrine:migrations:migrate

                    Application Migrations

WARNING! You are about to execute a database migration that could result in schema changes and data lost. Are you sure you wish to continue? (y/n)y
Migrating up to 20170729085036 from 20170703090821

  ++ migrating 20170729085036

     -> ALTER TABLE wallpaper ADD created_at DATETIME NOT NULL, ADD updated_at DATETIME NOT NULL

  ++ migrated (0.07s)

  ------------------------

  ++ finished in 0.07s
  ++ 1 migrations executed
  ++ 1 sql queries

Sweet.

There's one final thing we need to do before we get back to our WallpaperListener. We need to make sure to call setUpdatedAt() as part of a call to setFile.

Typically I don't test getter / setter logic on my entities. As such we don't have a PhpSpec file created for our Wallpaper entity. I'm going to start by generating one:

php vendor/bin/phpspec desc AppBundle/Entity/Wallpaper

Specification for AppBundle\Entity\Wallpaper created in /path/to/my/wallpaper/spec/AppBundle/Entity/WallpaperSpec.php.

Nothing new here.

This entity already exists so when we next run PhpSpec, we won't be prompted to create it. And as it happens, the generated test is passing:

php vendor/bin/phpspec run spec/AppBundle/Entity/WallpaperSpec.php
                                      100%                                       1
1 specs
1 example (1 passed)
26ms

This isn't surprising as the test checks that the class itself exists and has the expected type:

<?php

// spec/AppBundle/Entity/WallpaperSpec.php

namespace spec\AppBundle\Entity;

use AppBundle\Entity\Wallpaper;
use PhpSpec\ObjectBehavior;
use Prophecy\Argument;

class WallpaperSpec extends ObjectBehavior
{
    function it_is_initializable()
    {
        $this->shouldHaveType(Wallpaper::class);
    }
}

I'm not going to add in tests for every getter / setter. I'm just covering the interesting change. Feel free to go more in-depth here:

<?php

// spec/AppBundle/Entity/WallpaperSpec.php

namespace spec\AppBundle\Entity;

use AppBundle\Model\FileInterface;
use AppBundle\Entity\Wallpaper;
use PhpSpec\ObjectBehavior;
use Prophecy\Argument;

class WallpaperSpec extends ObjectBehavior
{
    function it_is_initializable()
    {
        $this->shouldHaveType(Wallpaper::class);
    }

    function it_sets_the_updated_at_timestamp_when_setting_a_file(
        FileInterface $file
    )
    {
        $this->getUpdatedAt()->shouldBe(null);

        $this
            ->setFile($file)
            ->shouldReturnAnInstanceOf(Wallpaper::class)
        ;

        $this
            ->getUpdatedAt()
            ->shouldReturnAnInstanceOf(\DateTime::class)
        ;
    }
}

Let's break this down.

First, before we do anything, the updatedAt class property should be null. This is because we don't set a default value anywhere for this property.

Next, we make our call to setFile. We pass in something that implements FileInterface, which we let PhpSpec worry about the fake creation thereof.

Whilst we don't need to check the return value of the setFile method, we might as well. Our setFile method returns $this, which will be the Wallpaper instance, so that's why we check that the return value of setFile is an instance of Wallpaper.

Finally, after calling setFile our assertion is that updatedAt should have been set, and so getUpdatedAt will return an instance of \DateTime. This logic isn't implemented yet, and so our test should fail:

php vendor/bin/phpspec run spec/AppBundle/Entity/WallpaperSpec.php

AppBundle/Entity/Wallpaper
  17  - it a
      expected an instance of DateTime, but got null.

                  50%                                     50%                    2
1 specs
2 examples (1 passed, 1 failed)
45ms

That's good.

Let's fix this:

class Wallpaper
{
    // * snip *

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

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

        return $this;
    }

Which gets us to a passing state:

php vendor/bin/phpspec run spec/AppBundle/Entity/WallpaperSpec.php
                                      100%                                       2
1 specs
2 examples (2 passed)
42ms

Nice.

This frees us up to do the final larger change which is to create the tests for preUpdate, and then fill in the missing implementation:

<?php

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

namespace spec\AppBundle\Event\Listener;

use AppBundle\Entity\Category;
use AppBundle\Entity\Wallpaper;
use AppBundle\Event\Listener\WallpaperListener;
use AppBundle\Service\FileMover;
use AppBundle\Model\FileInterface;
use AppBundle\Service\ImageFileDimensionsHelper;
use AppBundle\Service\WallpaperFilePathHelper;
use Doctrine\ORM\Event\LifecycleEventArgs;
use Doctrine\ORM\Event\PreUpdateEventArgs;
use PhpSpec\ObjectBehavior;
use Prophecy\Argument;

class WallpaperListenerSpec extends ObjectBehavior
{
    // * snip *

    function it_returns_early_if_preUpdate_LifecycleEventArgs_entity_is_not_a_Wallpaper_instance(
        PreUpdateEventArgs $eventArgs
    )
    {
        $eventArgs->getEntity()->willReturn(new Category());

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

        $this->fileMover->move(
            Argument::any(),
            Argument::any()
        )->shouldNotHaveBeenCalled();
    }

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

        $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->fileMover->move($fakeTempPath, $fakeNewFileLocation)->shouldHaveBeenCalled();

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

Oh my Lord, copy / paste gone wild!

It might not be immediately obvious here, but these two new tests are almost a direct copy / paste of the existing prePersist tests. There are some small but important changes:

• Changing the test names to reference preUpdate, not prePersist
• Change the test arguments to take a PreUpdateEventArgs instance
• Ensuring preUpdate is called, not prePersist

I'd rather not have a bunch of copy paste in my tests, but as ever there are trade offs to be made.

Right now the outcome - whether prePersist or preUpdate - is so similar that duplicating is the first, easiest step.

Could we refactor our test code to reduce the duplication? Yes, probably. But not simply.

I consider the duplication acceptable given that we have only two methods, and two tests for each. You may differ, and that's ok. Feel free to modify and adapt as needed. I choose not to make things more complex than they need be, at least when this early in a project.

At this stage we should have two failing tests:

php vendor/bin/phpspec run

AppBundle/Event/Listener/WallpaperListener
  92  - it returns early if preUpdate LifecycleEventArgs entity is not a Wallpaper instance
      expected false, but got null.

AppBundle/Event/Listener/WallpaperListener
 106  - it can preUpdate
      no calls have been made that match:
        Double\AppBundle\Service\FileMover\P18->move(exact("/tmp/some.file"), exact("/some/new/fake/some.file"))
      but expected at least one.

                                  90%                                      9%    21
7 specs
21 examples (19 passed, 2 failed)
90ms

Fixing the implementation is easy enough:

<?php

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

namespace AppBundle\Event\Listener;

use AppBundle\Entity\Wallpaper;
use AppBundle\Service\FileMover;
use AppBundle\Service\ImageFileDimensionsHelper;
use AppBundle\Service\WallpaperFilePathHelper;
use Doctrine\ORM\Event\LifecycleEventArgs;
use Doctrine\ORM\Event\PreUpdateEventArgs;

class WallpaperListener
{
    /**
     * @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)
    {
        return $this->upload(
            $eventArgs->getEntity()
        );
    }

    public function preUpdate(PreUpdateEventArgs $eventArgs)
    {
        return $this->upload(
            $eventArgs->getEntity()
        );
    }

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

        /**
         * @var $entity Wallpaper
         */

        $file = $entity->getFile();

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

        // got here
        $this->fileMover->move(
            $file->getPathname(),
            $newFileLocation
        );

        $this->imageFileDimensionsHelper->setImageFilePath($newFileLocation);

        $entity
            ->setFilename(
                $file->getFilename()
            )
            ->setHeight(
                $this->imageFileDimensionsHelper->getHeight()
            )
            ->setWidth(
                $this->imageFileDimensionsHelper->getWidth()
            )
        ;

        return $entity;
    }
}

And with that, we have fixed our failing tests:

php vendor/bin/phpspec run

                                      100%                                       21
7 specs
21 examples (21 passed)
46ms

We now have a tested, working Wallpaper Create and Update facility.

There's still work to do to bring us up to parity with our untested approach, so let's keep up the pace and get this fixed for good.