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

feat: Controller.php Add custom Config\Validation class #8908

Closed
wants to merge 16 commits into from

Conversation

daycry
Copy link
Contributor

@daycry daycry commented May 23, 2024

I think it is interesting to be able to add the custom configuration class, when you are using modules for example

Description
Explain what you have changed, and why.

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

I think it is interesting to be able to add the custom configuration class, when you are using modules for example
@daycry daycry changed the title Feature: Custom Validation class feature: Controller.php Add custom Validation class May 23, 2024
@daycry daycry changed the title feature: Controller.php Add custom Validation class feat: Controller.php Add custom Validation class May 23, 2024
@kenjis
Copy link
Member

kenjis commented May 23, 2024

Thank you for sending this PR!

We expect the following in all Pull Requests (PRs).

Important

We expect all code changes or bug-fixes to be accompanied by one or more tests added to our test suite to prove the code works.

If pull requests do not comply with the above, they will likely be closed. Since we are a team of volunteers, we don't have any more time to work on the framework than you do. Please make it as painless for your contributions to be included as possible.

See https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/pull_request.md

@kenjis kenjis added wrong branch PRs sent to wrong branch tests needed Pull requests that need tests enhancement PRs that improve existing functionalities docs needed Pull requests needing documentation write-ups and/or revisions. labels May 23, 2024
@kenjis
Copy link
Member

kenjis commented May 23, 2024

to add the custom configuration class, when you are using modules for example

What do you mean?
Can you explain an example use case in more detail.

@daycry
Copy link
Contributor Author

daycry commented May 23, 2024

By working in a modular way, each module is independent, and instead of adding validations in Config\Validation in each module I have a Validation class that extends Config\Validation.

Ex:
use Config\Validation;

class ApiValidation extends Validation
{
}

Loading my custom class validation:

Config(ApiValidation::class)

@kenjis kenjis changed the title feat: Controller.php Add custom Validation class feat: Controller.php Add custom Config\Validation class May 23, 2024
@kenjis
Copy link
Member

kenjis commented May 24, 2024

This is a breaking change.
See https://3v4l.org/jhv30

@kenjis kenjis added the breaking change Pull requests that may break existing functionalities label May 24, 2024
@kenjis
Copy link
Member

kenjis commented May 24, 2024

@daycry Thank you for the explanation. Indeed, the use case is likely.

@daycry
Copy link
Contributor Author

daycry commented May 24, 2024

What do I need to do?

@kenjis
Copy link
Member

kenjis commented May 24, 2024

See #8908 (comment)

  1. You sent this PR to the wrong branch (develop). You must send to 4.6 branch.
  2. You must write test code to prove the enhancement works.
  3. You need to update the user guide. Add short description to the changelog v4.6.0 and upgrading guide for v4.6.0, and update the descriptions for the updated methods.

But I'm not sure this PR (this implementation) should be merged.
Is there another better way to implement this feature?
I want to hear from other members.

@datamweb
Copy link
Contributor

We have been recommending using validateData() for the past few months, Shield uses this method frequently.
I think it involves a lot of software.

We need to see what other people think about this breaking change.

@kenjis
Copy link
Member

kenjis commented May 25, 2024

Yes, BREAKING CHANGE should be avoided as much as possible.
So it is better to implement features like this without breaking change.

@kenjis
Copy link
Member

kenjis commented May 25, 2024

How about adding a property for the validation classname?
Like $validationConfig = Validation::class.

@daycry
Copy link
Contributor Author

daycry commented May 25, 2024

How about adding a property for the validation classname? Like $validationConfig = Validation::class.

Maybe creating a public method allowing set the new attribute Validation like this.

Private Validation $config;

Public method setValidation(Validation $config){
$this->config = $config;
}

Replacing config('Validation') with this attribute.

Maybe is better.

@daycry
Copy link
Contributor Author

daycry commented Jun 4, 2024

I've done some changes for delete breaking change tag

@kenjis kenjis added GPG-Signing needed Pull requests that need GPG-Signing and removed breaking change Pull requests that may break existing functionalities labels Jun 5, 2024
Comment on lines 163 to 166
protected function setConfigValidator(Validation $config): void
{
$this->configValidation = $config;
}
Copy link
Member

Choose a reason for hiding this comment

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

Do we really need this method?
If there is a protected property, we can set it in our controllers.

$this->request = $request;
$this->response = $response;
$this->logger = $logger;
$this->configValidation = config(Validation::class);
Copy link
Member

@kenjis kenjis Jun 5, 2024

Choose a reason for hiding this comment

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

It is better that $configValidation is nullable, and if it is not null, set Config\Validation::class in setValidator().
Because when we don't use validation in the controller, we don't need the Config\Validation instance.

*
* @var Validation
*/
protected $configValidation;
Copy link
Member

@kenjis kenjis Jun 5, 2024

Choose a reason for hiding this comment

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

We use typed properties.

Suggested change
protected $configValidation;
protected ?Validation $configValidation = null;

@daycry
Copy link
Contributor Author

daycry commented Jun 5, 2024

Maybe, this method isn't necessary because It can set in every controllers using $this->configValidator = config...

@neznaika0
Copy link
Contributor

Why do you need a new config? Just to add rules? There is a better solution and it does not depend on the system.

  1. Create files in app/Validation/RuleGroups
  2. Add rules as methods
  3. Check rules in validator
    if ($this->validateData($input, UserRules::signin())) { ... }

Example app/Validation/RuleGroups/UserRules.php:

<?php

declare(strict_types=1);

namespace App\Validation\RuleGroups;

class User
{
    /**
     * Login form
     */
    public static function signin(): array
    {
        return [
            'email' => [
                'label' => 'User.email',
                'rules' => 'required|valid_email',
            ],
            'password' => [
                'label' => 'User.password',
                'rules' => 'required|min_length[6]',
            ],
        ];
    }
}

You can apply any rules separately from the system, have separate files for each action (instead of 100 properties in Validation). All you need is to add new Validation types $ruleSets or $templates

@daycry
Copy link
Contributor Author

daycry commented Jun 6, 2024

Thank you! I didn't know this.
Always I have think that all rules must write in Config\Validation.php.

@neznaika0
Copy link
Contributor

And an important advantage: you can execute any code in the method.

This is not always possible for a class property.
For example, you can use parameters from settings, dynamic translations, and various string glues...

@daycry
Copy link
Contributor Author

daycry commented Jun 6, 2024

And an important advantage: you can execute any code in the method.

This is not always possible for a class property. For example, you can use parameters from settings, dynamic translations, and various string glues...

An example, please?

I always use custom rules for check complexes data object or something like this.

@neznaika0
Copy link
Contributor

Hmm, array variants:
'rules' => 'if_exist|in_list[' . Switcher::YES->value . ']',
Or
'rules' => sprintf('in_list[%s]', $languages),
Or dynamic vars
'rules' => 'if_exist|' . CaptchaRules::getCaptchaRule(),

@daycry
Copy link
Contributor Author

daycry commented Jun 6, 2024

Hmm, array variants: 'rules' => 'if_exist|in_list[' . Switcher::YES->value . ']', Or 'rules' => sprintf('in_list[%s]', $languages), Or dynamic vars 'rules' => 'if_exist|' . CaptchaRules::getCaptchaRule(),

Aaahhh ok ok

I do this thinks some times.

Thank you for your help!

@kenjis
Copy link
Member

kenjis commented Jun 7, 2024

The feature to set validation rules in Config\Validation is from an very old version of CI,
but it does not seem to be a good feature in the current situation.

Instantiating a Config class that extends BaseConfig is very expensive. Because it must check Environment Variables for all properties. If you have 100 properties for validation rules, it must check the 100 Environment Variables (and probably all variables do not exist). So it is better not to have a lot of properties in the Config class.

Also, when using the feature, we specify the rule as a string (property name), so we cannot jump to the rule definition on an IDE.

If we use a simple static class that returns a rule array, it does not check unnecessary Environment Variables and we can jump to the method on an IDE. And we can put the class in anywhere.

@daycry
Copy link
Contributor Author

daycry commented Jun 8, 2024

I checked the code of CI and now is very flexible check data, I like It!

I Will close this PR.

Thank you for your help!

@daycry daycry closed this Jun 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs needed Pull requests needing documentation write-ups and/or revisions. enhancement PRs that improve existing functionalities GPG-Signing needed Pull requests that need GPG-Signing tests needed Pull requests that need tests wrong branch PRs sent to wrong branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants