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

Eloquent models isGuardableColumn() calls fail on models that have a schema name in the table property #33822

Closed
samoldenburg opened this issue Aug 11, 2020 · 7 comments

Comments

@samoldenburg
Copy link

samoldenburg commented Aug 11, 2020

  • Laravel Version: 6.18.35
  • PHP Version: 7.2.24
  • Database Driver & Version: mysql 5.7

Description:

I've been using Laravel in a complex database setup pretty successfully for a while now by specifying the database name in my models' $table properties.

    protected $table = 'schema_name.table_name';

As of #33777, all of my models that have defined a $guarded property instead of $fillable have stopped working with mass assignment if the model also specifies a schema name in it's $table property.

I've been successfully using eloquent in this setup for a couple years now, as it is much easier than defining all of the connections individually (I'm using Laravel to interface with up to 25 schemas on a single server). We also have cross schema relationships (legacy data is fun), and this all works wonderfully prior to #33777 having been merged.

I realize this is not a documented feature, and may not even be officially supported, but it does make my life a lot easier and this genuinely feels like a bug to me.

Steps To Reproduce:

  • Create a model that specifies a schema name in the $table property
  • Attempt mass assignment on the model. If you use a non-nullable database field you'll error out right away with an exception, something like xxx column doesn't have a default value.

Specifically I did the following

  • $ composer create-project --prefer-dist laravel/laravel=6.18.8 issue-test
  • configure a mysql database connection (database set to issuetest)
  • add two schemas to the database server (issuetest, and issuetest2)
  • $ php artisan make:migration create_test_tables
    public function up()
    {
        Schema::create('test1', function (Blueprint $table) {
            $table->bigIncrements('id');
            $table->string('unguarded');
            $table->timestamps();
        });

        Schema::create('issuetest2.test2', function (Blueprint $table) {
            $table->bigIncrements('id');
            $table->string('unguarded');
            $table->timestamps();
        });
    }
  • DB should now look something like:
    image
  • $ php artisan make:model Test1 && php artisan make:model Test2
class Test1 extends Model
{
    protected $guarded = ['id', 'created_at', 'updated_at'];
    protected $table = 'test1';
}
class Test2 extends Model
{
    protected $guarded = ['id', 'created_at', 'updated_at'];
    protected $table = 'issuetest2.test2';
}
  • Then it can be tested with tinker easily enough
    image

Potential Fix?

The issue appears to be all the way down in Illuminate\Database\Schema\MySqlBuilder. The getColumnListing() method does not work for tables denoted by schema.table.

The following change works for both models, but I'm sure there's a better way to write it, this was just a quick test. I'm also sure there's angles I'm not thinking of here.

    /**
     * Get the column listing for a given table.
     *
     * @param  string  $table
     * @return array
     */
    public function getColumnListing($table)
    {
        $table = $this->connection->getTablePrefix().$table;
        $dot = strpos($table, '.');
        if ($dot !== false) {
            $database = substr($table, 0, $dot);
            $table = substr($table, $dot + 1);
        } else {
            $database = $this->connection->getDatabaseName();
        }

        $results = $this->connection->select(
            $this->grammar->compileColumnListing(), [$database, $table]
        );

        return $this->connection->getPostProcessor()->processColumnListing($results);
    }

Thanks for taking a look.

@driesvints
Copy link
Member

Your model has a $connection property specifically to set this. Does that not work for you?

@samoldenburg
Copy link
Author

samoldenburg commented Aug 11, 2020

The only issue with that seems to be that I would need to add 20+ new config items to my config/database.php file, and then update all of my models accordingly (500+). I was concerned about relationships not working, but after a quick test it seems like they work just fine. I only tested a simple one to many relationship though.

My thinking for this being a bug is that the $table property has worked with schema_name.table_name for a while now. There are many references to this being an option outside of the official documentation on laracasts forums/stackoverflow questions. It is not specified in the official documentation far as I can see, though, so I wouldn't be surprised to hear that this just isn't a supported thing.

@driesvints
Copy link
Member

We've decided that we won't pursue backwards compatibility for this undocumented and unintended feature sorry. You should be able to get the behavior you want by overwriting the isGuarded method perhaps.

@samoldenburg
Copy link
Author

That's disappointing, but thanks for taking a look.

@zhuston
Copy link

zhuston commented Sep 28, 2020

We ran into this issue as well. After using laravel for many years I hadn't realized that specifying a different schema/database name on the table property wasn't documented. We ended up setting our uses of guarded to an empty array or removing them instead of switching to separate connections per schema. I did take note that we likely will need to switch to multiple connections at some point since specifying the schema with the table name isn't officially supported.

@driesvints
Copy link
Member

@zhuston you can still use the connection property for that

@zhuston
Copy link

zhuston commented Sep 29, 2020

@driesvints Thank you.

Primarily I was looking for the path of least resistance in order to get through with the upgrade. Our app has roughly 100 models that span 2 schemas/databases (same host). I had some concern that relations across separate connections might have issues. (they work fine now with database.tableName on the table property). I thought I read some people had issues with whereHas across connections since it creates a subquery?

I also had a minor concern for doubling up our connections to our database server with each request opening 2 connections. That would certainly be solvable if it was an issue, but for now it seemed the the easiest path was changing our usage of guarded.

Thank you for all of your work on Laravel!

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

3 participants