laravel / framework

The Laravel Framework.
https://laravel.com
MIT License
32.39k stars 10.98k forks source link

Request rule validation doesn't work with multi-level key based array config/database file #26573

Closed samdevbr closed 5 years ago

samdevbr commented 5 years ago

In order to validate data against database using rule validations, you may specify the correct database to use, unique:connection.table.

Theoretically we would have a database config file like this.

    'connections' => [
        'cooldb1' => [...]
    ]

And we would setup the rule like this unique:cooldb1.table,column

BUT now comes the problem, if we have multi-level key based array to store db configs, something like this

    'connections' => [
        'mysql' => [
               'cooldb1' => [...],
               'cooldb2' => [...]
        ],
        'cassandra' => [...],
        'redis' => [...]
    ]

And try to setup the rule based on the your db config file, we should have something like this unique:mysql.cooldb1.table,column, but unfortunately it doesn't work, we get an Undefined index: driver error.

I tried to google the solve for this problem, but no luck with that, but as many developers i'm cursed to the eternity to debug and try to find the solution, and I figured out that the function (\Illuminate\Validation\Concerns\ValidatesAttributes::parseTable) that parses the table and the db config, exploding the "table string" if it has ".", and getting the 2 first values, as we can see below.

    /**
     * Parse the connection / table for the unique / exists rules.
     *
     * @param  string  $table
     * @return array
     */
    protected function parseTable($table)
    {
        return Str::contains($table, '.') ? explode('.', $table, 2) : [null, $table];
    }

Based on my rule unique:mysql.cooldb1.table,column, the result would be.

array:2 [
  0 => "mysql" // Database
  1 => "cooldb1.table" // Table
]

And when the validator passes the config to DatabaseManager, and the db manager tries to get the config from config/database.php, it well get mysql key, instead of ['mysql']['cooldb1'], and that's when we get Undefined index: driver, because mysql key doesn't have db configs, only another keys, and these keys has db configs

To solve this problem, I changed the function on the server with nano, the solution was.

    /**
     * Parse the connection / table for the unique / exists rules.
     *
     * @param  string  $table
     * @return array
     */
    protected function parseTable($table)
    {
        if (Str::contains($table, '.')) {
            $settings = explode('.', $table);
            $table = last($settings);

            unset($settings[count($settings) - 1]);

            $connection = implode('.', $settings);

            return [$connection, $table];
        }

        return [null, $table];
    }

I'm not sure if it's a known issue, or something, but I think would be cool if the validator could be able to parse multi-level key array configuration from config/database, I've tested with Laravel and Lumen, same problem and same fix.

If everything is ok, I would happily create PR's for illuminate/validation (for Lumen) and laravel/framework (for Laravel) with the fix.

driesvints commented 5 years ago

Heya, unfortunately we don't support this Laravel version anymore. Please upgrade to the latest version and check if your problem persists.

samdevbr commented 5 years ago

Okay.

rodrigopedra commented 5 years ago

Hi @SamDevBR , the database connections are not meant to be nested,

The default connections in teh config/database.php are just defaults, but you can change the connection names as needed. They match their driver types in the config/database.php to be succinct but the driver property is what determinate the driver to be used in the connection.

So in your example, you should declare the connections like this:

// config/database.php
<?php

return [
  // ...

  'connections'=> [
    'cooldb1'=> [ // connection named cooldb1 that uses the mysql driver
      'driver'=> 'mysql',
      // ...
    ],

    'cooldb2'=> [ // connection named cooldb2 that also uses the mysql driver
      'driver'=> 'mysql',
      // ...
    ],
  ],

  // ...
];

This way your rules would work as such:

// in a controller
public function store(Request $request) {
  $this->validate($request, [
    'coolfield1' => 'required|unique:cooldb1.cooltable1,field',
    'coolfield2' => 'required|unique:cooldb2.cooltable2,field',
  ]);
}
samdevbr commented 5 years ago

@rodrigopedra Yeah, I understand how the config file should be to validation rules work fine, as you can see in my examples i described two settings for the config file, one works fine and the other one doesn't, but the issue here is that Eloquent, Query Builder and others services that uses config/database.php file, can dump the configuration dinamcally, and my current config for database works fine with them, but only validation rule service has problems while reading the config file for the connection and parsing the table, I think that the validation service should accept a different setup for the db config file just as the other services in Laravel, since we have many array helpers to get any key from any kind of array, and many services doesn't have any problem with my current config/database.php, I just expected that validation rule service was able to do the same.

mfn commented 5 years ago

What's you use case nesting the configuring as you outlined in contrast to what @rodrigopedra suggested? What benefits do you have?

(ps: I'm surprised this even works, I use multi connections but as rodrigopedra described it)

samdevbr commented 5 years ago

@mfn Our config/database.php file has many different dbs with different drivers such as mysql, redis and cassandra, and for organization purpose, we splited dbs into driver types, for example mysql key has 3 different dbs, cassandra, many contact points, and datacenters, it works fine with everything but rule validators, and I think would be great if rule validators behaves just as the other services, why rule validators must deal in a different way with the config/database.php ?

I've tested with latest version of Laravel and same issue, new issue open here https://github.com/laravel/framework/issues/26578

rodrigopedra commented 5 years ago

Hi @SamDevBR I re-read your post and you are right, the first example didn't catch my attention, sorry.