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

[0.2] Allow extending of validation rules #443

Merged
merged 19 commits into from
Dec 15, 2022

Conversation

wychoong
Copy link
Contributor

this is a proof of concept on customising validation rules in hub. Current only applied for Product's collection.

now you can define more rules in config

# in config/getcandy-hub/product.php
<?php

use GetCandy\Models\CollectionGroup;

return [
    # ... 

    'validation' => [
        'variant.sku' => 'required|unique|min:8',
        'collections.*.group_id' => [
            'required',
            'distinct',
        ],
        'collections' => ['required' => true, 'array', function ($attribute, $value, $fail) {
            $missing = [];

            $collections = collect($value)->pluck('id', 'group_id');

            $groups = CollectionGroup::whereIn('id', [1, 2])
                ->select(['id', 'name'])
                ->get()
                ->pluck('name', 'id');

            $missing = $groups->diffKeys($collections)->all();

            if (filled($missing)) $fail("Require Collection Group from: " . implode(', ', $missing));
        }],
    ]
];

result
image

image

issue
the main problem with the current implementation is unable pass in Model instance to those additional rules. it can be done but I don't think is good approach.

@vercel
Copy link

vercel bot commented Jul 29, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
getcandy-2-docs ✅ Ready (Inspect) Visit Preview Dec 14, 2022 at 2:49PM (UTC)
lunar-docs ✅ Ready (Inspect) Visit Preview Dec 14, 2022 at 2:49PM (UTC)

@wychoong
Copy link
Contributor Author

@glennjacobs need your input on this

this is referring to this https://discord.com/channels/497326121441034240/916819091841486869/1001891823146520646

@glennjacobs
Copy link
Contributor

@alecritson thoughts on this?

@alecritson
Copy link
Collaborator

@glennjacobs I'm not against this but don't feel like the config is the correct place. Due to some validation using closures (like in the example above) this means that Laravel wouldn't be able to serialise the config for optimisation.

Would it not be better to provide a method on the component that dev's can tap into at the component level? Something like:

ProductEdit::extendValidation([
    'name' => 'required|min:50',
]);

@wychoong
Copy link
Contributor Author

wychoong commented Aug 3, 2022

@glennjacobs I'm not against this but don't feel like the config is the correct place. Due to some validation using closures (like in the example above) this means that Laravel wouldn't be able to serialise the config for optimisation.

Would it not be better to provide a method on the component that dev's can tap into at the component level? Something like:

ProductEdit::extendValidation([
    'name' => 'required|min:50',
]);

Fair point. Will look into this approach

@wychoong
Copy link
Contributor Author

wychoong commented Aug 3, 2022

now can extend validation by using component method

ProductCreate::extendValidation(
            [
                'variant.sku' => ['required', 'min:8'],
                'collections' => ['required', 'array', function ($product) {
                    return function ($attribute, $value, $fail) use ($product) {
                        $missing = [];

                        $collections = collect($value)->pluck('id', 'group_id');

                        $groups = CollectionGroup::whereIn('id', [1, 2])
                            ->select(['id', 'name'])
                            ->get()
                            ->pluck('name', 'id');

                        $missing = $groups->diffKeys($collections)->all();

                        if (filled($missing)) $fail($product->translateAttribute('name') . " Require Collection Group from: " . implode(', ', $missing));
                    };
                }],
            ]
        );

$product is able to pass from component by using app()->call(...)
refer: getExtendValidation method

@glennjacobs glennjacobs changed the base branch from main to develop August 4, 2022 10:50
@alecritson
Copy link
Collaborator

@wychoong It's looking promising, I can see it's still in draft, is there still work you want to do on it?

@wychoong
Copy link
Contributor Author

@wychoong It's looking promising, I can see it's still in draft, is there still work you want to do on it?

hey. I think in terms of implementation it is done. I kept it as draft is to get feedbacks and since this opens up a lot more validation, we also need to add few more changes to display the error message.
in

  • getSideMenuProperty hasError
  • @error in blade

@wychoong wychoong marked this pull request as ready for review August 27, 2022 18:24
@glennjacobs
Copy link
Contributor

@wychoong should we have documentation for this?

@wychoong
Copy link
Contributor Author

wychoong commented Sep 5, 2022

@wychoong should we have documentation for this?

added doc

@wychoong
Copy link
Contributor Author

wychoong commented Sep 5, 2022

@wychoong should we have documentation for this?

added doc

Awesome. Just Alec's question and then I think this can be merged?

hrmmm... you referring to this?
image

not sure why its pending or should i click the resolve converstation?


return collect($rules)->map(function ($rule) use ($parameters) {
if ($rule instanceof Closure) {
return app()->call($rule, $parameters);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this idea is taken from Filament on allowing injecting value for closure

packages/admin/src/helpers.php Outdated Show resolved Hide resolved
packages/admin/src/helpers.php Outdated Show resolved Hide resolved
@wychoong
Copy link
Contributor Author

wychoong commented Sep 5, 2022

figured it out that i need to submit my "review" comment 😓

@glennjacobs
Copy link
Contributor

@wychoong the VuePress config file has conflicts now. You have changed the indentation from 2 spaces to 4 spaces, any reason for this?

@wychoong
Copy link
Contributor Author

@wychoong the VuePress config file has conflicts now. You have changed the indentation from 2 spaces to 4 spaces, any reason for this?

I think that was because of my VSCode does that, can you help on this as I’m away from computer till the weekend

@wychoong
Copy link
Contributor Author

@glennjacobs it's ready for review

@glennjacobs glennjacobs added this to the Lunar 0.2.0 milestone Sep 29, 2022
@glennjacobs glennjacobs changed the title extend validation rules Allow extending of validation rules Sep 29, 2022
@alecritson alecritson changed the base branch from develop to main November 7, 2022 12:22
@glennjacobs glennjacobs changed the title Allow extending of validation rules [0.2] Allow extending of validation rules Nov 11, 2022
@alecritson alecritson merged commit a27ce4b into lunarphp:main Dec 15, 2022
@wychoong wychoong deleted the extend-validation branch January 13, 2023 11:54
@alecritson alecritson mentioned this pull request Feb 23, 2023
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.

3 participants