archtechx / virtualcolumn

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

Add getCustomColumns function #10

Closed stein-j closed 2 years ago

stein-j commented 2 years ago

Hello @stancl,

This PR adds the function getVirtualColumns, which allows you to define which attributs you want to set as virtual columns instead of defining your custom columns. There are two main reasons why this is useful:

1) Many times I've had issues because someone in our team would add a new column but not add it in the getCustomColumns which would mess things up.

2) We always have many more custom columns than virtual columns. So we sometimes end up with an array with 30 attributes inside.

If getVirtualColumns() is defined, it will ignore getCustomColumns. Which is like how protected $fillable = [];and protected $guarded = [];

stancl commented 2 years ago

We always have many more custom columns than virtual columns

How would this deal with the fact that Tenancy can add a new virtual column at any time?

stein-j commented 2 years ago

It's not a breaking change. You either use getVirtualColumns or gerCustomColumns. So if tenancy is using the original function, it still works as usual.

Note: I just realized I need to update the test before it gets accepted

stancl commented 2 years ago

Are you using this package without Tenancy?

And I'm not really sure if this is a feature we want to support. If you have e.g. 5 specific columns in a data, and everything else in normal columns, why not just use data->foo for your columns?

The main point of this package is to add support for using any attributes on the model, and internally storing the ones that don't have custom columns in a generic data column.

If you know what virtual columns you have, then it makes more sense to use the standard Laravel approach data->foo.

stein-j commented 2 years ago

I use this package in my Tenancy app but on other models than the Tenant model.

If this package it only to allow you to add any attributes dynamically than don't merge this,

I think it does makes sens, because you package is called "virtual column" not "infinite column" so it doesn't necessarily implies the list is infinite. Like Laravel attributes, should you "fillable" specific attributes or you allow all but only "guard" specific attributes ?

Also side note : If a you save a virtual attribute with the name of a relation, it messes up the Model because it'll load the attribute instead of the relation (which makes sens in some ways).

stancl commented 2 years ago

I may reconsider this in the future, but for now I think that there isn't enough reason for the package to support explicitly listing virtual columns. For that, Laravel's standard JSON attributes should be used. This package is just for being able to store anything on the model without worrying about the column's existence.