How I wanted to improve Laravel but only made it worse

Prologue

Laravel is a great PHP framework and we use it a lot at Tixel. However, it’s publicly known that nothing is perfect in this world and improvements can (and should) always be made.

3 weeks ago we tried to make one little improvement to the testing capabilities of Laravel by creating two pull-requests (#1 and #2). Both got rejected but our attempts did prompt Taylor (Laravel author) to implement what we wanted. But, oh boy, the implementation that came through is awful.

Proposed new API for testing Mailables

Context

Integration testing is our preferred way of testing software because it strikes a nice balance between the cost of implementation and confidence in continuous deployment. Very often as a part of an integration test, we did some actions on behalf of a user and then made sure that Laravel sent an email. Laravel has a standard way to do this:

public function test_orders_can_be_shipped()
{
    Mail::fake();

    Mail::assertSent(OrderShipped::class);
}

This is good, but what if our Mailable class has a bit of logic inside and can render different texts based on it? Eg. “Order failed because of a credit card decline”, “Order failed because the product is out of stock” and so on.

We can pass a callback to the Mail fake:

Mail::assertSent(OrderShipped::class, function ($mail) use ($user) {
    return $mail->hasTo($user->email) &&
           $mail->somePublicProperty == 'someValue';
});

But all we can check here are public properties of our Mailable and To/Cc/Bcc fields (why are these methods even here by the way?). The Mailable class is implemented in such a way that there is no way to access rendered views that are going to be sent. This is what happens when you call render() on Mailable:

public function render()
{
    return $this->withLocale($this->locale, function () {
        Container::getInstance()->call([$this, 'build']);
        return Container::getInstance()->make('mailer')->render(
            $this->buildView(), $this->buildViewData()
        );
    });
}

It finds the current Laravel mailer and calls render() on it (so it’s a proxy method pretty much) passing semi-rendered data by calling buildView() and buildViewData() — protected methods you cannot access in your tests.

So if you want to test the email contents, your options are:

• Extend Mailable class so that you can access protected methods. Then all your Mailables have to extend this new class. If all developers in the world follow this path — this is a hell lot of repetition.
• Add Macroable trait to Mailable class so that it can be extended dynamically during testing. Mailable has a magic handler for methods starting with “with” and Macroable trait doesn’t support masks/regular expressions at the moment so it’s problematic to go this way.
• Create a proxy class that extends given Mailable class, it should forward all function calls and pass all public properties in case the user is testing these, plus add a handful of testing-related methods on top: seeInHtml(), seeInText() and so on. This is the option that I chose as the least intrusive.

Proposed solution

My proposed solution (second iteration) used Mockery to extend Mailable class during tests only — dynamically (monkey patching) adding extra methods:

public function email_confirmation_is_correct()
{
    Mail::fake();

    event(new TestEvent());
    config(['app.name' => 'Test App']);

    Mail::assertSent(TestMail::class, function (TestMail $mail) {
        return $mail->hasTo('test@test.com') 
          && $mail->seeInHtml('shipped')
          && $mail->dontSeeInHtml('failed') 
          && $mail->seeInText('Test App');
        });
    }

This looks simple, resembles Laravel’s Http Testing a lot and the only changed files are Testing namespace files — the production Mailable class is completely untouched.

It felt like a decent solution but then the pull-request was rejected but shortly Taylor made a tweet where he announced his own implementation of the same:

I was happy to see this — even though my code wasn’t merged, my actual proposed feature got released — hooray! The only difference in the API, looking at this screenshot, is the “assert” prefix in the method names.

Then I decided to look in the code, curious how he implemented it without Mockery or monkey patching…

The terrible

Obviously, quality of any software code is highly subjective but I’ll still try to elaborate on why I think Taylor’s implementation is terrible.

Taylor modified production class — Mailable, the class I felt uncomfortable touching, but it’s his framework so it’s fair, he knows what to do best.

He added these assert methods directly on Mailable:

/**
 * @param  string  $string
 * @return void
 */
public function assertSeeInText($string)
{
    [$html, $text] = $this->renderForAssertions();

    PHPUnit::assertTrue(
        Str::contains($text, $string),
        "Did not see expected text [{$string}] within text email body."
    );
}

Which is kind of wrong in my opinion:

• For most other classes/features in Laravel, testing-related functionality is nicely separated, kept in their own namespace and classes. While it may not be a huge deal, it’s kind of nice when things are sorted. Ideally, I would prefer mail-testing functionality to be either in MailFake class or in some kind of Test Mailable class
• Mailable class now references PHPUnit which is a development-only Composer dependency. So a production class is going to have a development import, on thousands of servers worldwide

But the worst part is this:

[$html, $text] = $this->renderForAssertions();

It seems that we are testing is not our normal renders, the renders we are going to email out. We are actually creating “special” renders for these assertions! Our aim was to test the actual production code — render() and send() methods, instead, we are testing a newly created method that didn’t exist before and that’s not being used for real-life rendering.

Let’s peek inside this new method:

protected function renderForAssertions()
{
    if ($this->assertionableRenderStrings) {
        return $this->assertionableRenderStrings;
    }

    return $this->assertionableRenderStrings = $this->withLocale($this->locale, function () {
        Container::getInstance()->call([$this, 'build']);

        $html = Container::getInstance()->make('mailer')->render(
            $view = $this->buildView(), $this->buildViewData()
        );

        $text = $view['text'] ?? '';

        if (! empty($text) && ! $text instanceof Htmlable) {
            $text = Container::getInstance()->make('mailer')->render(
                $view['text'], $this->buildViewData()
            );
        }

        return [(string) $html, (string) $text];
    });
}

Well, it’s a monster, isn’t it.

Not only it’s hard to follow, it repeats a lot of code from the real render() method, so now there is a chance that these two will get out of sync. And every time somebody changes render() method, they will have to think of this extra method and edit it as well.

I think it’s just strategically wrong — testing newly created test-specific methods instead of actual code that does the job. It defeats the purpose of automated testing, really.

I think with this commit, my favorite PHP framework got a bit worse.