Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[8.x] Fix bug where columns would be guarded while filling Eloquent models during unit tests #39880

Merged

Conversation

robbieaverill
Copy link
Contributor

@robbieaverill robbieaverill commented Dec 3, 2021

Versions

laravel/framework: v8.74.0 (affects versions in 6.x and up)
PHP: 7.4.21

Description

#33777 introduces a change that caches Eloquent model database table columns in memory to determine whether they should be guardable attributes or not.

This change does not consider whether or not a database connection is available. If it is unavailable, an empty list of columns will be cached for that model, resulting in subsequent tests that do have a database connection treating all columns as unfillable.

Example to reproduce

Create a unit test, and an integration test:

<?php

namespace Tests\Unit;

use MyProject\Product;
use Tests\TestCase;

/**
 * @group guardedbug
 */
class EloquentModelFillingTest extends TestCase
{
    public function testFillModel()
    {
        $foo = new Product(['name' => 'testing', 'price' => 10]);
        $this->assertSame('testing', $foo->name);
    }
}
<?php

namespace Tests\Feature;

use MyProject\Product;
use Illuminate\Foundation\Testing\RefreshDatabase;
use Tests\TestCase;

/**
 * @group guardedbug
 */
class EloquentSavingTest extends TestCase
{
    use RefreshDatabase;

    public function testFillAndSaveModel()
    {
        $foo = new Product();
        $foo->fill(['name' => 'testing', 'price' => 10]);
        $foo->save();
        $this->assertSame('testing', $foo->name);
    }
}

When you run these tests individually, the unit test fails, and the integration test passes:

# --filter EloquentModelFillingTest
1) Tests\Unit\EloquentModelFillingTest::testFillModel
Failed asserting that null is identical to 'testing'.

# --filter EloquentSavingTest
.                                                                   1 / 1 (100%)
OK (1 test, 1 assertion)

When you run them in the same test suite, they both fail. This is because the unit test run caches the empty list of columns in memory:

# --group guardedbug
Tests: 2, Assertions: 1, Errors: 1, Failures: 1.

(The error is because my environment has name as a non-nullable column in my table).

Proposal

This pull request proposes that we do not cache the column list when it is empty. This means that when running tests that don't have access to a database, it won't interfere with the rest of a suite where tests may in future have access to a database.

With this patch both of these tests pass:

# --group guardedbug
OK (2 tests, 2 assertions)

I realise this change was made to address some security issues (https://blog.laravel.com/security-release-laravel-61834-7232, https://blog.laravel.com/security-release-laravel-61835-7240).

My workaround for now is to put Model::unguard() in TestCase::setUp(), however I thought I'd propose a fix here as well.

@robbieaverill robbieaverill changed the title Fix bug where columns would be guarded while filling Eloquent models during unit tests [8.x] Fix bug where columns would be guarded while filling Eloquent models during unit tests Dec 3, 2021
@derekmd
Copy link
Contributor

derekmd commented Dec 3, 2021

The forceFill method can also be used by unit tests to bypass database column checks:

public function forceFill(array $attributes)
{
return static::unguarded(function () use ($attributes) {
return $this->fill($attributes);
});
}

Product::make()->forceFill([
    'name' => 'testing',
    'price' => 10,
]);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants