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

[6.x] Allow explicit Model definitions in database rules #30653

Merged
merged 4 commits into from
Nov 22, 2019
Merged

[6.x] Allow explicit Model definitions in database rules #30653

merged 4 commits into from
Nov 22, 2019

Conversation

ahinkle
Copy link
Contributor

@ahinkle ahinkle commented Nov 22, 2019

This PR creates the ability to explicitly define models within Database Rule; allowing to pass models to the exists and unique validation rules.

$request->validate([
      'user_id' => 'required|exists:App\User,id'
])
$request->validate([
      'user_id' => 'required|unique:'.User::class.',id'
])

I've always found it a bit strange when I have to write out the database table name within my validation rules when the framework will give you a hand in most other places (such as eloquent relationships).

This will also prevent issues when you are changing up the table name and you don't have to go through each and every exists and unique rule to update the table name.

@mgrinspan
Copy link
Contributor

Does it make sense to extend on this and automatically determine which column to search through (if none is provided) based off of the model's primary key?

}

return $model->getTable();
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feel this could have been cleaner by checking the opposite, i.e. checking if the argument is a Model instance rather than not; checking the string is a class and exists rather than not.

public function resolveTableName($table)
{
    if (is_object($table) && $table instanceof Model) {
        return $table->getTable();
    }

    if (class_exists($table)) {
        return (new $table)->getTable();
    }

    return $table;
}

@martinbean
Copy link
Contributor

Does this work with the fluent rule builders too, i.e. Rule::unique(ModelName::class)?

@lindyhopchris
Copy link

@ahinkle great addition but think it should also be using $model->getConnectionName().

@clugg
Copy link
Contributor

clugg commented Nov 27, 2019

Unless I am missing something, I don't believe this PR is covering every possible case. Keeping in mind that my models are separated into the App\Models namespace:

use App\Models\User;

Option 1: validation rules as a single | delimited string

Input:

$request->validate([
    'alias' => 'string|unique:'.User::class,
]);

Output:
SQLSTATE[42S02]: Base table or view not found: 1146 Table 'homestead.App\\Models\\User' doesn't exist

Option 2: validation rules as an array of strings

Input:

$request->validate([
    'alias' => ['string', 'unique:'.User::class],
]);

Output:
SQLSTATE[42S02]: Base table or view not found: 1146 Table 'homestead.App\\Models\\User' doesn't exist

Option 3: validation rules as an array of Rule instances

Input:

use Illuminate\Validation\Rule;

$request->validate([
    'alias' => ['string', Rule::unique(User::class)],
]);

Output:
Works as expected, table is resolved properly and validation continues without errors.

I believe that this is because when using parsed validation rules, the flow of validation does not at any stage create new instances of Illuminate\Validation\Rules\Exists or Illuminate\Validation\Rules\Unique. Eventually, inside Illuminate\Validation\Validator, $this->$method is called, with $method being one of validateExists or validateUnique. At this point all of the validate* methods are within Illuminate\Validation\Concerns\ValidatesAttributes, including validateExists and validateUnique. These ultimately do not create new instances of the rule classes mentioned above, which means that the code in this PR is never executed and so the table name is never resolved from the model. I am a little confused about this, as the PR's example explicitly uses options 1 and 2, which don't work for me, while option 3 (which is not mentioned) does.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants