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

proposal: refactoring of options configuration #60

Open
wants to merge 1 commit into
base: 2.14.x
Choose a base branch
from

Conversation

codisart
Copy link
Contributor

Q A
QA yes

Description

This pull request is my proposal to refactor the options system for the validator objects.
I have two propositions.

The first one is to use a simple trait with a single public method to reduce the duplication of the code that transform function argumetns into an array of options

The second one is to use a full fleged object to do the transformation. It is in my opinion heavier to use but I think it could be the base of more refactoring of the options system. Maybe adding validation.

Signed-off-by: codisart <louis.celier@gmail.com>
@glensc
Copy link

glensc commented Aug 11, 2020

perhaps drop the trait, but rather add static methods to ValidatorOptionsObject for factory patterns.

@geerteltink geerteltink changed the base branch from master to 2.14.x September 12, 2020 07:47
@codisart
Copy link
Contributor Author

codisart commented Oct 1, 2020

Hello @glensc, thank you for the review.

I like your idea, what do you think of

$options = ValidatorOptions::asArray(['min', 'max', 'inclusive'], func_get_args());
class ValidatorOptions
{
    public static asArray(array $keys, array $args) {
        $this->shouldHaveOnlyStringValues($keys);

        $result = [];
        foreach ($args as $arg) {
            $result[array_shift($keys)] = $arg;
        }
        return $result;
    }

    private function shouldHaveOnlyStringValues(array $keys) : void
    {
        foreach ($keys as $key) {
            if (! is_string($key)) {
                throw new \InvalidArgumentException('The values of $keys should be only strings');
            }
        }
    }
}

@boesing boesing added the RFC label Nov 18, 2020
@boesing
Copy link
Member

boesing commented Nov 18, 2020

@codisart Could you please create an RFC issue instead of a PR?
I think we might want to have some more feedback on what we want to change and having an issue without a concrete implementation detail could engage more people in participating the discussion.

@codisart
Copy link
Contributor Author

Hello @boesing,

I never did a RFC issue. Do you have an exemple to follow ?

@boesing
Copy link
Member

boesing commented Nov 18, 2020

@codisart Sure, have a look at discourse for now, as we are trying to move RFCs to github issues since the november TSC meeting. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants