statamic / cms

The core Laravel CMS Composer package
https://statamic.com
Other
4.09k stars 534 forks source link

Custom Validation Rules not processed if no value in field #11151

Open jonathan-bird opened 1 day ago

jonathan-bird commented 1 day ago

Bug description

If you create a new custom validation rule and set it in a blueprint. Eg.

validate:
  - 'new App\Rules\TemplateDefaultOrNotSet()'

Then it won't run the validate() method unless there's a value set in the field.

It should be run regardless of whether there is a value.

How to reproduce

Create a new custom rule:

<?php

namespace App\Rules;

use Closure;
use Illuminate\Contracts\Validation\DataAwareRule;
use Illuminate\Contracts\Validation\ValidationRule;

class TemplateDefaultOrNotSet implements ValidationRule, DataAwareRule
{
    public function validate(string $attribute, mixed $value, Closure $fail): void
    {
        $fail('It will not hit this.');
     }
}

Then go to blueprint yaml file and set this (or just set new App\Rules\TemplateDefaultOrNotSet() in the Blueprint view):

validate:
  - 'new App\Rules\TemplateDefaultOrNotSet()'

And then once saved, create a new page and don't set any value in that field, and you will notice that it won't hit that validate() method (although it will hit a __construct() if set in the custom rule)

Logs

No response

Environment

Environment
Application Name: xxxxx
Laravel Version: 11.33.2
PHP Version: 8.3.13
Composer Version: 2.7.7
Environment: local
Debug Mode: ENABLED
URL: xxxx-statamic.test
Maintenance Mode: OFF
Timezone: Australia/Brisbane
Locale: en

Cache
Config: NOT CACHED
Events: NOT CACHED
Routes: NOT CACHED
Views: NOT CACHED

Drivers
Broadcasting: log
Cache: redis
Database: mysql
Logs: stack / daily
Mail: smtp
Queue: sync
Session: redis

Livewire
Livewire: v3.5.12

Statamic
Addons: 7
Sites: 1
Stache Watcher: Enabled (auto)
Static Caching: Disabled
Version: 5.38.1 PRO

Statamic Addons
aerni/advanced-seo: 2.9.3
aerni/livewire-forms: 9.4.1
digital-bird/statamic-toc: 1.0.2
jonassiewertsen/statamic-livewire: 3.8.1
mitydigital/sitemapamic: 3.2.0
mitydigital/statamic-two-factor: 2.3.0

Installation

Fresh statamic/statamic site via CLI

Additional details

No response

jasonvarga commented 23 hours ago

It should be run regardless of whether there is a value.

I don't think that's accurate. The field is only validated if it's included, or if you mark it as required.

Validator::make([
  // foo missing
], [
  'foo' => 'min:5',
])->validate();  // passes
Validator::make([
  'foo' => 'h',
], [
  'foo' => 'min:5',
])->validate();  // fails: the foo field must be at least 5 characters
Validator::make([
  'foo' => 'hello',
], [
  'foo' => 'min:5',
])->validate();  // passes
Validator::make([
  // foo missing
], [
  'foo' => 'required|min:5',
])->validate();  // fails: the foo field is required
jonathan-bird commented 21 hours ago

@jasonvarga this is kind of the point though, based on that logic you can't write your own required-type validation because the class can't even run a validate method. Why can't it pass through the field as null like what Laravel would do in a form?

Or do I have to set "always save" for that logic to apply?

Because of the fact the data isn't always saved, I'm stuck in a circumstance where it's hard to validate logic with the template field too where it's empty by default so it makes it difficult to validate if not set or someone has set it to default template.

Or I get the field to show sometimes and set "sometimes" validation and it's still not required despite the label saying it will be required when it shows.

Interested to hear your thoughts on the Statamic way, this is just how I approach things developing in Laravel.

jasonvarga commented 20 hours ago

If the field is hidden, it won't be submitted, and won't be validated.

That's consistent to traditional forms with Laravel. If a form is missing an input, it doesn't get submitted, and wouldn't be validated.

What visibility condition are you applying to it?

In order to force the field to be submitted/validated:

jonathan-bird commented 20 hours ago

@jasonvarga just to clarify, in my original post the field was set, it was just an empty value (was visible).

I'll take a look at your suggestions too

jasonvarga commented 19 hours ago

I'll eat my words now. You're absolutely right.

We add nullable to non-required fields behind the scenes.

https://github.com/statamic/cms/commit/f0ff892970880fefae2bb51bd7c71e9942775820

It's been like that for a while though. We'll have to think about how to resolve this in a non-breaking way.

jonathan-bird commented 19 hours ago

Haha all good! If I can help at all please let me know.

I thought I wasn't going crazy cause the screenshots from Marty a while back show it as working - https://www.martyfriedel.com/blog/how-to-use-custom-laravel-validationrule-rules-in-statamic

jasonvarga commented 19 hours ago

Those class based rules definitely work, but as you've pointed out, they need to have a value.

jasonvarga commented 19 hours ago

Actually thinking about it a little more, I think it's probably working fine.

Can you elaborate on what you actually wanted to do in your custom validation rule?

You've named it TemplateDefaultOrNotSet which sounds like maybe you're planning to allow the value to be null anyway, in which case it shouldn't matter if the rule doesn't run.

robdekort commented 3 hours ago

I think @freshface is running into a similar issue with frontend forms (precognition). Since you can't use required|sometimes currently due to #9172, he made a custom validation rule called RequiredIfArrayContains. This validation rule should make a field required if an array (coming from a checkbox field) contains a certain value. However this validation logic doesn't seem to trigger if the field doesn't have a value. And that's exactly when you'd want it to trigger. It only triggers on blur when there's already data in the field.

freshface commented 2 hours ago

You need to add public $implicit = true;

<?php

namespace App\Rules;

use Closure;
use Illuminate\Contracts\Validation\ValidationRule;
use Illuminate\Contracts\Validation\DataAwareRule ;

class RequiredIfArrayContains implements DataAwareRule, ValidationRule
{

    protected $data = [];
    public $implicit = true;

    public function __construct(
        private $key = null,
        private $containsValue = null,
    ) {
        //

    }

    /**
     * Run the validation rule.
     *
     * @param  \Closure(string): \Illuminate\Translation\PotentiallyTranslatedString  $fail
     */
    public function validate(string $attribute, mixed $value, Closure $fail): void
    {
        ray($this->key);
       if(isset($this->data[$this->key]) && in_array($this->containsValue, $this->data[$this->key])) {
         if(empty($value)) {
            $fail('The :attribute field is required.')->translate();
         }
        }
    }

    public function setData(array $data): static
    {
        $this->data = $data;

        return $this;
    }
}
jasonvarga commented 2 hours ago

Oh, there you go. Nice.

robdekort commented 2 hours ago

Neat!

freshface commented 19 minutes ago

It works, but would be nice to make the 'sometimes' option work again. Due to this limitation its not possible anymore for clients to build a form with "show if when condition x is met" anymore.