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

Virtual columns and Model::preventAccessingMissingAttributes #50946

Closed
ju5t opened this issue Apr 6, 2024 · 5 comments
Closed

Virtual columns and Model::preventAccessingMissingAttributes #50946

ju5t opened this issue Apr 6, 2024 · 5 comments

Comments

@ju5t
Copy link

ju5t commented Apr 6, 2024

Laravel Version

11.2.1

PHP Version

8.2

Database Driver & Version

MySQL 8.0.33

Description

In our app we use virtual generated columns. As Model::preventAccessingMissingAttributes is not aware of them, it throws an exception. There doesn't seem to be a supported solution for this in Laravel.

Steps To Reproduce

We have the following migration:

Schema::table('users', function (Blueprint $table) {
    $table->string('name_normalized')
        ->after('name')
        ->virtualAs("regexp_replace(name, '[^a-zA-Z0-9]', '')")
        ->index();
});

Accessing it with $user->name_normalized will throw the exception. We could use attributes for it, but doing it in MySQL has some benefits we prefer not to give up (e.g. search).

I didn't make a test repo as I thought the issue doesn't require one, but if it helps, let me know and I'll happily set one up.

@hafezdivandari
Copy link
Contributor

hafezdivandari commented Apr 6, 2024

Following test is passing using MySQL. What I'm missing here?

public function testVirtualColumnWithPreventAccessingMissingAttributes()
{
    Schema::create('test_model', function (Blueprint $table) {
        $table->id();
        $table->string('name');
    });

    Schema::table('test_model', function (Blueprint $table) {
        $table->string('name_normalized')
            ->after('name')
            ->virtualAs("regexp_replace(name, '[^a-zA-Z0-9]', '')")
            ->index();
    });

    $model = new class extends Model {
        protected $table = 'test_model';
        protected $guarded = ['id'];
        public $timestamps = false;
    };

    $model::query()->create(['name' => 'Foo Ba$r']);

    $model::preventAccessingMissingAttributes();

    $this->assertEquals('FooBar', $model->first()->name_normalized);
}

Perhaps you forgot to run your migrations php artisan migrate?
What is the output of Schema::getColumns('users')?

@ju5t
Copy link
Author

ju5t commented Apr 7, 2024

You're right, nothing wrong with that. I've done a few more tests and it seems be related to model factories and as such, only pops up in our tests.

$this->actingAs($user = User::factory()->create());
$name = auth()->user()->name_normalized;

Both $user and auth()->user() only have the attributes that were defined in the factory definition, this means the name_normalized attribute is missing.

The solution is to use:

$this->actingAs($user = User::factory()->create()->refresh());

As this reloads all attributes from the database, which includes the virtual generated one. I'm not sure what to make of this yet. One the one hand, this feels a bit like a workaround, on the other, I'm not sure if a factory should provide a 'refreshed' state automatically either.

Copy link

github-actions bot commented Apr 8, 2024

Thank you for reporting this issue!

As Laravel is an open source project, we rely on the community to help us diagnose and fix issues as it is not possible to research and fix every issue reported to us via GitHub.

If possible, please make a pull request fixing the issue you have described, along with corresponding tests. All pull requests are promptly reviewed by the Laravel team.

Thank you!

@plumthedev
Copy link
Contributor

@ju5t using refreshing on factory could lead to n+1 queries during tests but somehow make sense. Maybe model should be marked as something interface as HasVirtualColumns to refresh only these which are implementing this interface.

namespace Illuminate\Contracts\Database\Eloquent;

interface HasVirtualColumns
{}
namespace App\Models;

use Illuminate\Database\Eloquent\Model;
use Illuminate\Contracts\Database\Eloquent\HasVirtualColumns;

class User extends Model implements HasVirtualColumns
{}

This change should be reflected in documentation as well.

@taylorotwell
Copy link
Member

I'll be honest, I'm not sure how much longer I want to keep changing the framework to support a feature I don't like (barfing on "missing attributes"). The feature hasn't been documented in a while, and I would rather just tell people not to use it at all. It's not a great feature. It breaks packages. It breaks core framework features, and I'm getting weary of working around it with more and more edge-case PRs. 😅

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

No branches or pull requests

5 participants