archtechx / virtualcolumn

Eloquent Virtual Column package.
MIT License
70 stars 12 forks source link

Proposal autodetecting actual columns #20

Closed davidjr82 closed 4 months ago

davidjr82 commented 4 months ago

Hello!

I was testing this package as I am testing the great tenancy package.

The only thing that bothers me a bit is having to specify the actual columns of the model where I want to use this trait. I ended up using this in my own trait, so any real column is not included in the data column:

public static function getCustomColumns(): array
{
    return \Illuminate\Support\Facades\Schema::getColumnListing((new static)->getTable());
}

Is this something that makes sense to add by default in this package, instead of a default ['id'] array?

stancl commented 4 months ago

Wouldn't getColumnListing() run a SQL query each time this method is called?

davidjr82 commented 4 months ago

Yes, but it can be cached (I do it with 24hr or whenever I run migrations, so for me it is only one query per day and model).

I am using the package to allow my customers to add dynamic columns. If it would be only for the tenancy package, then it's not worth it.

So if the scope of the package is intended only for tenancy, I would reject the idea. But if it is a wider scope, then I think it is valuable to consider it.

stancl commented 4 months ago

I think the columns should be defined statically. That way they can be versioned along with migrations and we avoid the entire rabbit hole of:

If you want to get the column list dynamically from the database, you can extend the method like you have.