Add validator's priority retrieval into Factory #180

mtagliab · 2019-04-10T11:55:13Z

I noticed that it was impossible to set validator's priority using the "array notation", even if this feature was already there as it already exists for filters (see line 384 of Factory.php).

To add the priority, it will be enough to add the priority key in the array:

$inputFilter[] = [
    'name' => 'text_elt',
    'required' => true,
    'filters' => [
        ['name' => 'StripTags'],
        ['name' => 'StringTrim']
    ],
    'validators' => [
        [
            'name' => 'StringLength',
            'options' => [
                'max' => 256
            ],
            'priority' => 2
        ]
    ]
];

FactoryTest fix

I also noticed that the FactoryTest class had a minor issue, which was that validators where not tested inside the testFactoryWillCreateInputWithSuggestedValidators method.
I fixed the loop and added a final assert to verify that all the validators have been tested.

froschdesign · 2019-08-23T12:18:00Z

@webimpress
I think an unit test is missing here. Like this one:

zend-inputfilter/test/FactoryTest.php

Lines 685 to 726 in a23e67d

    
           public function testFactoryWillNotGetPrioritySetting() 
        
           { 
        
               //Reminder: Priority at which to enqueue filter; defaults to 1000 (higher executes earlier) 
        
               $factory = $this->createDefaultFactory(); 
        
               $input   = $factory->createInput([ 
        
                   'name'    => 'foo', 
        
                   'filters' => [ 
        
                       [ 
        
                           'name'      => 'StringTrim', 
        
                           'priority'  => Filter\FilterChain::DEFAULT_PRIORITY - 1, // 999 
        
                       ], 
        
                       [ 
        
                           'name'      => 'StringToUpper', 
        
                           'priority'  => Filter\FilterChain::DEFAULT_PRIORITY + 1, //1001 
        
                       ], 
        
                       [ 
        
                           'name'      => 'StringToLower', // default priority 1000 
        
                       ], 
        
                   ], 
        
               ]); 
        
               // We should have 3 filters 
        
               $this->assertEquals(3, $input->getFilterChain()->count()); 
        
               // Filters should pop in the following order: 
        
               // string_to_upper (1001), string_to_lower (1000), string_trim (999) 
        
               $index = 0; 
        
               foreach ($input->getFilterChain()->getFilters() as $filter) { 
        
                   switch ($index) { 
        
                       case 0: 
        
                           $this->assertInstanceOf(Filter\StringToUpper::class, $filter); 
        
                           break; 
        
                       case 1: 
        
                           $this->assertInstanceOf(Filter\StringToLower::class, $filter); 
        
                           break; 
        
                       case 2: 
        
                           $this->assertInstanceOf(Filter\StringTrim::class, $filter); 
        
                           break; 
        
                   } 
        
                   $index++; 
        
               } 
        
           }

The foreach on the chain won't return its validators. The method $chain->getValidators() return an array with extra data, so the instance need to be extracted. Asserts haven't been changed, but an additional assert assure that the foreach has been executed.

Test is a bit tricky, because when we get validators from ValidatorChain using getValidators method we are getting them as an array, not in order by priority. When validators are called (using isValid method) these are using the order they were added to the chain.

michalbundyra · 2019-08-23T17:33:56Z

@froschdesign test added. Would you mind to have a look on it, please?

It's a bit tricky as I mentioned in the commit comment - we can't use getValidators on ValidatorChain, as the result is not sorted by priority. When validators are called (in isValid) these are ordered, and this is how I check it.

froschdesign · 2019-08-23T17:54:03Z

@webimpress

When validators are called (in isValid) these are ordered, and this is how I check it.

👍

mtagliab · 2019-08-24T19:32:40Z

src/Factory.php

-                if (isset($validator['priority'])) {
-                    $priority = $validator['priority'];
-                }
+                $priority = isset($validator['priority']) ? $validator['priority'] : ValidatorChain::DEFAULT_PRIORITY;


Even if this is the most logic way to do it, I used the "ugly" format to maintain coherence with the upper statement (breakChainOnFailure).
If you want to use the ternary operator, it would be nicer to re-format the previous check too, so there aren't different coding styles in the same file.

@mtagliab
I've changed it to match the logic from populateFilters:

zend-inputfilter/src/Factory.php

Line 384 in c29cc31

$priority = isset($filter['priority']) ? $filter['priority'] : FilterChain::DEFAULT_PRIORITY;

As you can see the coding style is mixed in the file...

Actually, you are right, I didn't see that.

Add validator's priority retrieval into Factory

Forward port #180

michalbundyra · 2019-08-28T19:23:18Z

Thanks, @mtagliab!

michalbundyra added the bug label Aug 23, 2019

michalbundyra added this to the 2.10.1 milestone Aug 23, 2019

michalbundyra added the unit test needed label Aug 23, 2019

mtagliab added 2 commits August 23, 2019 15:27

Added priority on validator attac

e51ea94

michalbundyra changed the base branch from develop to master August 23, 2019 14:28

michalbundyra added 2 commits August 23, 2019 18:28

Use one linear statement - ternary operator

d14441f

michalbundyra removed the unit test needed label Aug 23, 2019

michalbundyra requested review from froschdesign and weierophinney August 23, 2019 17:34

froschdesign approved these changes Aug 23, 2019

View reviewed changes

mtagliab commented Aug 24, 2019

View reviewed changes

michalbundyra added a commit that referenced this pull request Aug 28, 2019

Merge pull request #180 from mtagliab/validator-priority-factory

691d8d4

Add validator's priority retrieval into Factory

michalbundyra added a commit that referenced this pull request Aug 28, 2019

Adds CHANGELOG entry for #180

bc04696

michalbundyra merged commit cc1effe into zendframework:master Aug 28, 2019

michalbundyra added a commit that referenced this pull request Aug 28, 2019

Merge branch 'hotfix/180' into develop

5feeeff

Forward port #180

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add validator's priority retrieval into Factory #180

Add validator's priority retrieval into Factory #180

Uh oh!

mtagliab commented Apr 10, 2019

Uh oh!

froschdesign commented Aug 23, 2019

Uh oh!

michalbundyra commented Aug 23, 2019

Uh oh!

froschdesign commented Aug 23, 2019

Uh oh!

mtagliab Aug 24, 2019

Uh oh!

michalbundyra Aug 24, 2019

Uh oh!

mtagliab Aug 28, 2019

Uh oh!

michalbundyra commented Aug 28, 2019

Uh oh!

Uh oh!

Add validator's priority retrieval into Factory #180

Add validator's priority retrieval into Factory #180

Uh oh!

Conversation

mtagliab commented Apr 10, 2019

FactoryTest fix

Uh oh!

froschdesign commented Aug 23, 2019

Uh oh!

michalbundyra commented Aug 23, 2019

Uh oh!

froschdesign commented Aug 23, 2019

Uh oh!

mtagliab Aug 24, 2019

Choose a reason for hiding this comment

Uh oh!

michalbundyra Aug 24, 2019

Choose a reason for hiding this comment

Uh oh!

mtagliab Aug 28, 2019

Choose a reason for hiding this comment

Uh oh!

michalbundyra commented Aug 28, 2019

Uh oh!

Uh oh!