ZF-8693: BUG! Incorrect use switch(true)

Description

Bug in Zend/File/Transfer/Adapter/Abstract.php

method addValidators;

    $validators = array(
        array('MimeType',  true, array('image/jpeg', 'image/gif', 'image/png')),
        array('FilesSize', true, array('max' => '1MB', 'messages' => 'файл больше 1MБ')),
    );

    $this->_upload->addValidators($validators, 'img');

set validators for all files, because error in block

                    switch (true) {
                        case (0 == $argc):
                            break;
                        case (1 <= $argc):
                            $validator  = array_shift($validatorInfo);
                        case (2 <= $argc):
                            $breakChainOnFailure = array_shift($validatorInfo);
                        case (3 <= $argc):
                            $options = array_shift($validatorInfo);
                        case (4 <= $argc):
                            $files = array_shift($validatorInfo);
                        default:
                            $this->addValidator($validator, $breakChainOnFailure, $options, $files);
                            break;

Must be changed to:

                    if ($argc > 0) {
                        $validator  = array_shift($validatorInfo);

                        if (2 <= $argc)
                            $breakChainOnFailure = array_shift($validatorInfo);
                        if (3 <= $argc)
                            $options = array_shift($validatorInfo);
                        if (4 <= $argc)
                            $files = array_shift($validatorInfo);

                        $this->addValidator($validator, $breakChainOnFailure, $options, $files);
                    }

Invalid use switch See on oficial site php.net

<?php switch ($i) { case 0: echo "i equals 0"; case 1: echo "i equals 1"; case 2: echo "i equals 2"; } ?>

Here, if $i is equal to 0, PHP would execute all of the echo statements! If $i is equal to 1, PHP would execute the last two echo statements. You would get the expected behavior ('i equals 2' would be displayed) only if $i is equal to 2. Thus, it is important not to forget break statements (even though you may want to avoid supplying them on purpose under certain circumstances).

Comments

Code is not correct. Test again, because this code always run case 3 and 4 - set files to null always.

$validators = array( array('MimeType', true, array('image/jpeg', 'image/gif', 'image/png')), array('FilesSize', true, array('max' => '1MB', 'messages' => 'файл больше 1MБ')), );

$this->_upload->addValidators($validators, 'img');

Test this!!! Case 1 - true always, and not has break - run cases 2-4. This is error!!!

We don't need to remove the switch statement; we need to merely change the conditions. switch(true) is a perfectly valid use case.

Please provide a reproduce case and expected and actual results, please.

I think code below should work correctly


switch ($argc) {
    default:
        $extraParams = array_slice($validatorInfo, 4, $argc - 4); // It's not clear for me what to do with extra parameters. But we have kept them just in case
        $validatorInfo = array_slice($validatorInfo, 0, 4);
    case 4:
        $files = array_pop($validatorInfo);
    case 3:
        $options = array_pop($validatorInfo);
    case 2:
        $breakChainOnFailure = array_pop($validatorInfo);
    case 1:
        $validator = array_pop($validatorInfo);
        $this->addValidator($validator, $breakChainOnFailure, $options, $files);
    case 0:
    break;
}

same code as diff


@@ -400,19 +400,20 @@
                         $options   = $validatorInfo;
                         $this->addValidator($validator, $breakChainOnFailure, $options, $files);
                     } else {
-                        switch (true) {
-                            case (0 == $argc):
-                                break;
-                            case (1 <= $argc):
-                                $validator  = array_shift($validatorInfo);
-                            case (2 <= $argc):
-                                $breakChainOnFailure = array_shift($validatorInfo);
-                            case (3 <= $argc):
-                                $options = array_shift($validatorInfo);
-                            case (4 <= $argc):
-                                $files = array_shift($validatorInfo);
+                        switch ($argc) {
                             default:
+                                $extraParams = array_slice($validatorInfo, 4, $argc - 4); // It's not clear for me what to do with extra parameters. But we have kept them just in case
+                                $validatorInfo = array_slice($validatorInfo, 0, 4);
+                            case 4:
+                                $files = array_pop($validatorInfo);
+                            case 3:
+                                $options = array_pop($validatorInfo);
+                            case 2:
+                                $breakChainOnFailure = array_pop($validatorInfo);
+                            case 1:
+                                $validator = array_pop($validatorInfo);
                                 $this->addValidator($validator, $breakChainOnFailure, $options, $files);
+                            case 0:
                                 break;
                         }
                    }

Sergey: Again... please provide a reproducable use.

The above code works in about 5 components. When it has to be changed it would mean that 5 components do not work properly. Therefor please add a usecase which shows where you have problems in your application.

Just changing the core without knowing why is not a good practice.

Thomas Weidner: Sorry, but reporter of issue Sergey and I am (Sergey Boroday) are two different "Sergey". :)

Author of issue http://framework.zend.com/issues/secure/…

But you right. Shame me. Proposed code didn't fix anything and will not work correctly.

If I use validators in this format

$validators = array( array('MimeType', true, array('image/jpeg', 'image/gif', 'image/png')), array('FilesSize', true, array('max' => '1MB', 'messages' => 'файл больше 1MБ')), );

I set 3 parameters ($argc)

4 parameter - null, I add validator to file key, for example 'img'

$upload->addValidators($validators, 'img');

But bug in case set validators to all files!!!! What do you do not understand????

case (4 <= $argc): $files = array_shift($validatorInfo); ------ null!!!

Case must be changed to if, or inverse case order from max $argc = 4 to 1

I change code to

                    if ($argc > 0) {
                        $validator = array_shift($validatorInfo);

                        if (2 <= $argc)
                            $breakChainOnFailure = array_shift($validatorInfo);

                        if (3 <= $argc)
                            $options = array_shift($validatorInfo);

                        if (4 <= $argc)
                            $files = array_shift($validatorInfo);

                        $this->addValidator($validator, $breakChainOnFailure, $options, $files);
                    }

успокойся. Им нужен детальный кейс для воспроизведения ситуации. А еще лучше тест.

I tried to write test but here some questions


    public function testAdapterShouldAllowAddingMultipleValidatorsAtOnceUsingBothInstancesAndPluginLoaderForDifferentFiles()
    {
        $validators = array(
            array('MimeType', true, array('image/jpeg')), // no files
            array('FilesSize', true, array('max' => '1MB', 'messages' => 'файл больше 1MБ')), // no files
            array('Count', true, array('min' => 1, 'max' => '1', 'messages' => 'файл не 1'), 'bar'), // 'bar' from config
            array('MimeType', true, array('image/jpeg'), 'bar'), // 'bar' from config
        );

        $this->adapter->addValidators($validators, 'foo'); // set validators to 'foo'

        $test = $this->adapter->getValidators();
        $this->assertEquals(4, count($test)); 

        //test files specific validators
        $test = $this->adapter->getValidators('foo');
        $this->assertEquals(4, count($test)); // how many validators expect? 2 from 'no files' or 4 'no files' and overwrited 'bar'?
        $mimeType = array_shift($test);
        $this->assertTrue($mimeType instanceof Zend_Validate_File_MimeType);
        $filesSize = array_shift($test);
        $this->assertTrue($filesSize instanceof Zend_Validate_File_FilesSize);

        $test = $this->adapter->getValidators('bar');
        $this->assertEquals(0, count($test)); // how many validators expect? 2 from 'bar' or 0 because we overwrite 'bar' with 'foo'
        $filesSize = array_shift($test);
        $this->assertTrue($filesSize instanceof Zend_Validate_File_FilesSize);
        $mimeType = array_shift($test);
        $this->assertTrue($mimeType instanceof Zend_Validate_File_MimeType);

        $test = $this->adapter->getValidators('baz');
        $this->assertEquals(0, count($test)); 

    }

Fixed with r20206.

Some notes: * The provided code snippets where not used (they added other problems) * The provided unittests are partitially wrong * Actually it is not possible to set multiple instances of the same class... one MimeType instance overwrites the other... this is also mentioned within the manual (will be fixed by another issue)

                if (is_string($name)) {
                    $validator = $name;
                    $options   = $validatorInfo;
                    $this->addValidator($validator, $breakChainOnFailure, $options, $files);
                } else {
                    if ($argc > 0) {
                        $validator = array_shift($validatorInfo);

                        if (2 <= $argc)
                            $breakChainOnFailure = array_shift($validatorInfo);
                        if (3 <= $argc)
                            $options = array_shift($validatorInfo);
                        if (4 <= $argc)
                            $files = array_shift($validatorInfo);

                        $this->addValidator($validator, $breakChainOnFailure, $options, $files);
                    }
                }

I want use format 2

    $validators = array(
        array('MimeType',  true, array('image/jpeg', 'image/gif', 'image/png')),
        array('FilesSize', true, array('max' => '1MB', 'messages' => 'файл больше 1 MБ')),
    );

equivalent to format 1

    $validators = array(
        'MimeType'  => array('image/jpeg', 'image/gif', 'image/png'),
        'FilesSize' => array('max' => '1MB', 'messages' => 'файл больше 1 MБ'),
    );

Because format 1 has always $breakChainOnFailure = false!!!

It would be a pleasure if you also read what you get responded by people:

{quote} •Actually it is not possible to set multiple instances of the same class... one MimeType instance overwrites the other... this is also mentioned within the manual (will be fixed by another issue) {quote}

Additionally the syntax of your format2 works and is tested within our unittests.

HOW IT WORK?????

Your code always set $files to null!!!!

switch (true) { case (0 == $argc): break; case (1 <= $argc): $validator = array_shift($validatorInfo); case (2 <= $argc): $breakChainOnFailure = array_shift($validatorInfo); case (3 <= $argc): $options = array_shift($validatorInfo); case (4 <= $argc): $files = array_shift($validatorInfo); default: $this->addValidator($validator, $breakChainOnFailure, $options, $files); break;

ALWAYS case 4! always $files = null in my example

Sergey. Please read comments carefully. Issue fixed. Get the new version from trunk.