[3.x] Using Rule classes (or their macros) instead of the registered rules can result in invalid queries

ipontt-neering commented 1 month ago

PHP & Platform

8.3

Database

MySQL 8

Laravel version

11.x

Have you done this?

[X] I have checked my logs and I'm sure is a bug in this package.
[X] I can reproduce this bug in isolation (vanilla Laravel install)
[X] I can suggest a workaround as a Pull Request

Expectation

The expectation is that registered rules (rut_unique:users) should behave the same as Rule classes (or macros) (Rule::rutUnique/ new Laragear\Rut\Rules\RutUnique(...)) and generate the same queries.

Description

These are the deprecation warnings and exception thrown.

DEPRECATED  str_contains(): Passing null to parameter #1 ($haystack) of type string is deprecated in vendor/laravel/framework/src/Illuminate/Database/Query/Builder.php on line 887.

DEPRECATED  stripos(): Passing null to parameter #1 ($haystack) of type string is deprecated in vendor/laravel/framework/src/Illuminate/Database/Grammar.php on line 86.

DEPRECATED  str_contains(): Passing null to parameter #1 ($haystack) of type string is deprecated in vendor/laravel/framework/src/Illuminate/Database/Grammar.php on line 177.

DEPRECATED  explode(): Passing null to parameter #2 ($string) of type string is deprecated in vendor/laravel/framework/src/Illuminate/Database/Grammar.php on line 97.

Illuminate\Database\QueryException  SQLSTATE[42S22]: Column not found: 1054 Unknown column '' in 'where clause' (Connection: mysql, SQL: select exists(select * from `users` where `rut_num` = 11111111 and UPPER(users.rut_vd) = 1 and `` = id) as `exists`).

This is the "path" I observed for both methods when the validate method gets called on the validator. rut_unique:users

Laragear\Rut\ValidatesRut::validateRutUnique(“rut”, “11.111.111-1”, [“users”], Validator)
Laragear\Rut\ValidatesRut::query(“rut”, Rut, [“users”], Validator)
Laragear\Rut\ValidatesRut::parseParameters([“users”], 3, 5)
Laragear\Rut\ValidatesRut::addExtraWheres(Builder, [null, null])

Rule::rutUnique(User::class)

Laragear\Rut\ValidatesRut::validateRutUnique(“rut”, “11.111.111-1”, [“App\Models\User”, “NULL”, “NULL”, “NULL”, “id”], Validator)
Laragear\Rut\ValidatesRut::query(“rut”, Rut, [“App\Models\User”, “NULL”, “NULL”, “NULL”, “id”], Validator)
Laragear\Rut\ValidatesRut::parseParameters([“App\Models\User”, “NULL”, “NULL”, “NULL”, “id”], 3, 5)
Laragear\Rut\ValidatesRut::addExtraWheres(Builder, [null, “id”])

I've managed to fix the error locally by changing the addExtraWheres method from.

    protected static function addExtraWheres(Builder $query, array $wheres): Builder
    {
        foreach (array_chunk($wheres, 2) as $item) {
            if ($item[1]) {
                $query->where($item[0], $item[1]);
            }
        }

        return $query;
    }

to

    protected static function addExtraWheres(Builder $query, array $wheres): Builder
    {
        foreach (array_chunk($wheres, 2) as $item) {
            if ($item[1] && $item[0] !== null) {
                $query->where($item[0], $item[1]);
            }
        }

        return $query;
    }

Reproduction

use App\Models\User;
use Illuminate\Support\Facades\DB;
use Illuminate\Validation\Rule;

// listen for queries and dump them as they happen (or log them using the Log facade)
DB::listen(fn ($query) => dump([$query->sql, $query->bindings]));

// These 2 validators should behave the same.
$v1 = validator(['rut' => '11.111.111-1'], ['rut' => ['rut_unique:users']]);
$v2 = validator(['rut' => '11.111.111-1'], ['rut' => Rule::rutUnique(User::class)]);

$v1->validate(); // works as expected
$v2->validate(); // throws a bunch of deprecation warnings and fails with a QueryException

Stack trace & logs

No response

ipontt-neering commented 1 month ago

Rule::numUnique works well enough for my use case as it is currently,

DarkGhostHunter commented 1 month ago

Rule::numUnique works well enough for my use case as it is currently,

That rules essentially does Rule::unique() but on the RUT number column (rut_num by default).

I guess I'll have to take a deeper look into the these rules once I have time, so I'll leave this open.

DarkGhostHunter commented 4 weeks ago

Okay, I took a look at it and the problem is simple: The ignore column parameter "id" should be NULL string when there is no id to ignore.

If there is an ID to ignore, it should be set as "id" column name if the user didn't set it, or whatever the user has set. If the User passes a Model instance, it should be the primary key.

Since the rule does remove all arguments that are NULL, we can simply follow the aforementioned logic rules to set RutUnique::$idColumn to NULL unless needed.

Laragear / Rut