Skip to content
This repository has been archived by the owner on Jan 30, 2020. It is now read-only.

Added test to check if possible to set country with context #42

Merged

Conversation

svycka
Copy link
Contributor

@svycka svycka commented Apr 14, 2016

I guess this was desired behavior but didn't worked.

this is extremely useful for example with input filters, example:

class User extends InputFilter
{
    public function __construct()
    {
        $this->add([
            'name'       => 'country_code',
            'required'   => true,
            'validators' => [
                new CountryCode()
            ],
        ]);
        $this->add([
            'name'       => 'phone',
            'required'   => true,
            'validators' => [
                [
                    'name' => 'PhoneNumber',
                    'options' => [
                        'country' => 'country_code',
                    ]
                ],
        ]);
    }
}

before I had to wrap PhoneNumber into callback validator and pass country in constructor.

@@ -3164,10 +3164,12 @@ public function testAllowPossibleSetterGetter()
$this->assertTrue($this->validator->allowPossible());
}

public function testSetCountryMethodIsCaseInsensitive()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know changing tests is not good but maybe not a BC break? Anyway getCountry() is used only in one place and I covered it so maybe not a problem?

Copy link
Member

Choose a reason for hiding this comment

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

Your changes make sense. We'll want to be able to retrieve the original version, but also validate in a case-insensitive manner. This is a good solution.

@svycka
Copy link
Contributor Author

svycka commented Apr 14, 2016

workaround example:

class User extends InputFilter
{
    public function __construct()
    {
        $this->add([
            'name'       => 'country_code',
            'required'   => true,
            'validators' => [
                new CountryCode()
            ],
        ]);
        $this->add([
            'name'       => 'phone',
            'required'   => true,
            'validators' => [
                'name' => 'Zend\Validator\Callback',
                'options'  => array(
                    'messages' => [
                        \Zend\Validator\Callback::INVALID_VALUE => 'Invalid phone number.',
                    ],
                    'callback' => function ($phone, $context) {

                        $phoneValidator = new PhoneNumber([
                            'country' => $context['country_code'],
                        ]);
                        return $phoneValidator->isValid($phone);
                    },
                ),
            ],
        ]);
    }
}

@weierophinney weierophinney added this to the 2.7.3 milestone Jun 7, 2016
@weierophinney weierophinney self-assigned this Jun 7, 2016
@weierophinney weierophinney merged commit 37b8325 into zendframework:master Jun 7, 2016
weierophinney added a commit that referenced this pull request Jun 7, 2016
Added test to check if possible to set country with context
weierophinney added a commit that referenced this pull request Jun 7, 2016
weierophinney added a commit that referenced this pull request Jun 7, 2016
weierophinney added a commit that referenced this pull request Jun 7, 2016
@weierophinney
Copy link
Member

Thanks, @svycka

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

Successfully merging this pull request may close these issues.

2 participants