[ericmp33]

#56346 (comment)

Should either be all or nothing

So lets go with all!

[ericmp33]

For rules that only implement the __toString method, we could directly return the string instead of having a dedicated class.

On one hand, I feel it's easier to make the method grow if it's in its own class. But if the class has no extra methods and only returns a string, it probably makes more sense to return the string directly.

Let me know if I should change it.

---

Edit: changed on 891cd8f

[shaedrich]

Ah, sorry, marked my review as a duplicate because GitHub accidentally displayed it twice—now, I can't unhide it 😬

#56405 (review)

[ericmp33]

Yeah, maybe, this should be done via __call() 🤔

How would you do it? Can you give me an example, @shaedrich?

[shaedrich]

For rules that only implement the __toString method, we could directly return the string instead of having a dedicated class.

Yeah, maybe, this should be done via __call() 🤔

How would you do it? Can you give me an example, @shaedrich?

Sure thing:

class Rule
{
    use Macroable;
+
+   const SIMPLE_RULES = [
+       'accepted',
+       'active_url',
+       'ascii',
+       // …
+   ];

    /**
     * Get a can constraint builder instance.
    // …
        return $parser->explode(ValidationRuleParser::filterConditionalRules($rules, $data));
    }
+
+   public function __call(string $method, array $arguments)
+   {
+       if (in_array($method, self::SIMPLE_RULES)) {
+           return $method;
+       }
+   }
}

This could be extended with shortcuts to rule classes:

    public function __call(string $method, array $arguments)
    {
        if (in_array($method, self::SIMPLE_RULES)) {
            return $method;
-       }
+       } else if (class_exists(($ruleClass = __NAMESPACE__ . '\\Rules\\' . Str::studly($method)))) {
+          return new $ruleClass;
+       }
    }

[shaedrich]

Would be nice if we had a base rule where we could just pass the rule and its arguments and it would generate the string for us

Would be nice if we don't have to define fluent setters for every rule but would only have to define the options (like we do for commands) and let the base rule do the rest

I feel the method chaining it's clear and makes sense for this case, doesn't it? What do you mean "only have to define the options (like we do for commands) and let the base rule do the rest"?

The AbstractRule class

abstract class AbstractRule
{
    /**
     * @var  array<string, bool>  $flags
     */
    private array $flags = [];

    /**
     * @var  array<string, null|scalar|scalar[]>  $options
     */
    private array $options = [];

    public function __construct(...$arguments)
    {
        if (Arr::every($arguments, fn ($value, int|string $key) => is_string($key))) {
            throw new InvalidArgumentException('Please use named arguments');
        }

        foreach ($arguments as $key => $value) {
            if (array_key_exists($key, $this->options)) {
                $this->options[$key] = array_unique([...$this->options[$key], $value]);
            } else if (array_key_exists($key, $this->flags) && is_bool($value)) {
                $this->flags[$key] = $value;
            }
        }
    }

    public function __call(string $method, array $arguments)
    {
        if (array_key_exists($method, $this->options) && count($arguments)) {
            $this->options[$method] = array_unique(array_merge($this->options[$method], $arguments));
        } else if (array_key_exists($method, $this->flags)) {
            $this->flags[$method] = true;
        }
    }

    public function __toString(): string
    {
        return collect([
            Str::of(class_basename())->snake(),
            ...collect($this->flags)->filter()->keys(),
            ...collect($this->options)->filter()->map(fn ($value, string $key) => $key . ':' . (is_array($value) ? implode(',', $value) : $value)),
        ])->join('|');
    }
}

[ericmp33]

#56405 (comment) -> Don't you think we are overcomplicating it?

I feel the same here - #56405 (comment) - but I understand we might want to refactor my code somehow.

[shaedrich]

I can see why you think that. In the end, it comes down to what someone sees as complicated: Redundant but expressive code or compact code with a lot of indirection ("magic"). I'd say, both have their respective valid arguments.

[ericmp33]

I can see why you think that. In the end, it comes down to what someone sees as complicated: Redundant but expressive code or compact code with a lot of indirection ("magic"). I'd say, both have their respective valid arguments.

Yeah that's exactly what I think

[shaedrich]

Fingers crossed! :)

[taylorotwell]

Don't think I'm going to pursue this right now but will probably explore cleaner validation in Laravel 13.