Skip to content

[12.x] Feat: add validation rule builders for all rules #56405

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

Draft
wants to merge 12 commits into
base: 12.x
Choose a base branch
from

Conversation

ericmp33
Copy link

#56346 (comment)

Should either be all or nothing

So lets go with all!

- Implement individual rule classes for all validation rules
- Rename ArrayRule to Arr for consistency
- Add tests for new rule classes
- Update Rule class to support new rule instances
@ericmp33 ericmp33 marked this pull request as draft July 24, 2025 19:35
@ericmp33 ericmp33 marked this pull request as ready for review July 24, 2025 19:58
@ericmp33
Copy link
Author

ericmp33 commented Jul 28, 2025

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.

@taylorotwell taylorotwell marked this pull request as draft August 3, 2025 15:29
shaedrich

This comment was marked as duplicate.

@shaedrich
Copy link
Contributor

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

#56405 (review)

@ericmp33
Copy link
Author

ericmp33 commented Aug 5, 2025

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

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

@shaedrich
Copy link
Contributor

shaedrich commented Aug 5, 2025

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
Copy link
Contributor

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('|');
    }
}

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.

2 participants