laravel / ideas

Issues board used for Laravel internals discussions.
938 stars 28 forks source link

[Proposal] Eloquent Model's table variable using static #945

Open yidas opened 6 years ago

yidas commented 6 years ago

Hello,

I'm considering to change Eloquent Model's table variable to static, as following points:

  1. Each Model only need a static table name
  2. Better way to get each Model's table name with Model::getTable() (https://github.com/laravel/framework/issues/1436)
  3. Less memory slightly comparing with renew Model itself to get table

For now, Model::getTable() wouldn't be work with __callStatic because there has non-static public getTable() block in front.

My suggested pattern as following:

<?php

echo A::getTable(). "\n"; // A
echo B::getTable(). "\n"; // B
echo C::getTable(). "\n"; // Cs (Guessing)

class Model
{
    protected static $table;

    public static function getTable()
    {
        if (!isset(static::$table)) {
            return str_replace(
                '\\', '', Str::snake(Str::plural(class_basename($this)))
            );
        }

        return static::$table;
    }
}

class A extends Model
{
    protected static $table = 'A';
}

class B extends Model
{
    protected static $table = 'B';
}

class C extends Model
{

}

BTW, getTable() method could be static or not.

Thanks

deleugpn commented 6 years ago

I really like your proposal. One thing I hate is when I'm writing a Form Request and I want to validate a unique rule, I need to specify the table name and there is no easy way to delegate that responsibility to the Eloquent Model instead (unless you instantiate it). Your proposal would allow me to do User::getTable(). However, keep in mind that similar changes have been proposed, accepted and reverted (see https://github.com/laravel/framework/pull/22478 and https://github.com/laravel/framework/pull/22734). If you could test if your method does not break assertEquals as described by themsaid would be cool.

yidas commented 6 years ago

Thanks, @deleugpn

I know that this change is hard to apply because it changes the way for table variable setting (protected $table => protected static $table).

Glad to discuss with you.

misog commented 6 years ago
  1. Each Model only need a static table name

Consider this: You have a huge table in your relational database (statistics, chat messages, ...), let it be stats table and Stats model. One a year you move old records to an archive table, ex. table stats_2016 to speed up processing of records in the main stats table - you do not need old data for this, but they should be accessible if needed.

You implemented model/trait setter method, ex. Stats::setTable() which replaces the $table property just for the current model object - you call the method if you need to access the old data.

But if the $table property is static, it will replace the value for all model objects which is not desired.

  1. Less memory slightly comparing with renew Model itself to get table

I think the complexity of implementation and non-universality of the design is not worth it for the slight memory savings.

sisve commented 6 years ago

One a year you move old records to an archive table, ex. table stats_2016 to speed up processing of records in the main stats table - you do not need old data for this, but they should be accessible if needed.

Such examples require me to mentioned table partitioning. There's no reason to have tables split up like this; you can just tell your database engine to store 2016 in a separate file (which could be on a slower spinning harddrive if necessary), and it will not be touched at all when you query for data in other time periods. https://dev.mysql.com/doc/refman/5.7/en/partitioning-overview.html

halaei commented 5 years ago

I think a lot of model properties can be static:

That is possibly some memory waste. Maybe if someone really needs them to not being static, they can override non-static getter functions the way they want to.

mfn commented 5 years ago

Currently all of these properties could be changed during runtime.

It's currently possible to have two instances of a models behaving differently because you can change them "at object runtime".

With such a (super-breaking) change, this woulnd't be possible anymore.

I'm not saying people should do that or it's a good practice, but how do you propose to keep this functionality for those who make use of it?

staudenmeir commented 5 years ago

I agree with @misog, $table and the other properties should stay non-static.

Besides partitioning, there are other scenarios where custom tables per model instance are useful/necessary. I use them to select data from views or common table expressions.

The ability to change the database connection dynamically is also an important feature. There's even a dedicated method for it.

halaei commented 5 years ago

@mfn I don't think it is even possible to be 100 percent backward compatible. However, if all the static properties have non-static getters, people can override them to use non-static properties if they have to. Still I don't get why the fields were not defined static in the first place. Why the memory waste, especially in cases of working with large collections of models in not considered a big deal at all? Perhaps in case of working with big enough volume of data, you are better off not using eloquent but using raw DB query builder instead?

halaei commented 5 years ago

@staudenmeir Thanks for sharing your reasons. I still think the same level of flexibility for special cases can be achieved by other solutions, e.g. inheritance or method override.

halaei commented 5 years ago

Also every version of laravel adds a couple of more non static properties to model, like it is not a big deal. I don't know, maybe not a big deal 🙄

mfn commented 5 years ago

especially in cases of working with large collections

That's the one thing I try to avoid because I'm aware of the overhead.

PipStyles commented 3 years ago

(sorry to necro, just thinking on this myself having written a workaround) I think backwards compatibility may be possible with something like this in Model...

public static $tableName;

protected $table;

public function getTable()
{
  return $this->table 
         ?? static::$tableName 
         ?? str_replace( '\\', '', Str::snake(Str::plural(class_basename($this))));
}

This way, dynamic override at runtime is accommodated and takes priority, but the static can be used to set the default table name if the model convention doesn't apply.