nothingworksinc / ticketbeast

Back to the lesson videos:
https://course.testdrivenlaravel.com/lessons
552 stars 11 forks source link

[1.3 - Getting to Green] Model returns null for incorrect column #73

Closed debasishrowlo closed 6 years ago

debasishrowlo commented 6 years ago

The "assertSee" method asserts the view data fine but it is failing in situation like repeated texts or incorrect column name.

// Test
$concert = factory(Concert::class)->states('published')->create([
            'title' => 'The Red Chord',
            'subtitle' => 'The Red Chord',
            ...
]);
<!-- View -->
{{ $concert->title }}
{{ $concert->subtitles }}

In this case the "assertSee" for "subtitle" will pass while the "subtitles" will be rendered null. Is there any way to throw an error instead of silently returning null when incorrect column name is typed in a view?

adamwathan commented 6 years ago

Hey @debasishrowlo! Yeah this is an unfortunate consequence of how lenient Eloquent is about property access like that.

If you wanted to avoid this, you could make it a personal rule to never access Eloquent properties that way from outside the model, and create dedicated methods for getting all of that data like $concert->title() and $concert->subtitle().

Personally I find it to be a bit overkill and just accept that templates generally are more prone to mistakes that are hard to catch with automated tests. It tends to work out okay for me in the end because if I'm building a template, I have it open in the browser as well, so it's very rare to not catch a mistake like that.

pdbreen commented 6 years ago

To catch this, I created a StrictAttributes trait I add to models throws an exception/logs an error if a given $key cannot be resolved. There are a few places where this trait won't work (some eloquent constructs), but has definitely prevented a number of errors for me.

trait StrictAttributes
{
    public function getRelationValue($key)
    {
        // If the key already exists in the relationships array, it just means the
        // relationship has already been loaded, so we'll just return it out of
        // here because there is no need to query within the relations twice.
        if ($this->relationLoaded($key)) {
            return $this->relations[$key];
        }

        // If the "attribute" exists as a method on the model, we will just assume
        // it is a relationship and will load and return results from the query
        // and hydrate the relationship's value on the "relationships" array.
        if (method_exists($this, $key)) {
            return $this->getRelationshipFromMethod($key);
        }

        $msg = 'Strict attribute exception: '.$key;
        if (config('app.debug')) {
            throw new \Exception($msg);
        }
        \Bugsnag::notifyError('error', $msg);
    }
}
debasishrowlo commented 6 years ago

Raised this issue because I recently changed some poorly named column names in one of my projects. After that the UI was a mess.

Thanks @pdbreen. Your solution uncovered all old columns in views.

Just wanted to know your thoughts before closing, @adamwathan.