Presence of FileField breaks FunctionalTest::submitForm

Description

Affects at least framework 4.5.3 & cms 4.5.1 - but I imagine the issue affects every version. Tested with CWP release 2.5.2 that uses core release 4.5.2.

Have form with dropdown fields
End to end test form for business rules around submission data using FunctionalTest::submitForm
add new file field from silverstripe/assets to accomodate new requirement
every test now fails

Steps to Reproduce

Tested via composer create-project silverstripe/recipe-cms formtesttest 4.5.2 The form must have both DropdownField and FileField present. Other fields appear to be irrelevant. Each field separately sees the tests pass. Note no field has any custom validation, nor does the form.

Clarification: the issue exists only in automated testing envrionment (Siverstripe's phpunit test harness). Manual reproduction attempts (font-end user testing) see everything work as expected.

<?php

namespace {

    use SilverStripe\Forms\DropdownField;
    use SilverStripe\Forms\FieldList;
    use SilverStripe\Forms\FileField;
    use SilverStripe\Forms\Form;
    use SilverStripe\Forms\FormAction;
    use SilverStripe\View\SSViewer;
    use SilverStripe\CMS\Controllers\ContentController;

    class PageController extends ContentController
    {
        private static $allowed_actions = ['Form', 'FormPass'];

        public function Form()
        {
            return Form::create(
                $this,
                __FUNCTION__,
                FieldList::create(
                    DropdownField::create('Breaks', null, [
                        'one' => 'First option',
                        'two' => 'Second option',
                    ]),
                    FileField::create('uploaded')
                ),
                FieldList::create(
                    FormAction::create('formSubmission', 'Submit')
                )
            );
        }

        public function FormPass()
        {
            $form = $this->Form()->setName(__FUNCTION__);
            $form->Fields()->removeByName('uploaded');
            return $form;
        }

        public function formSubmission()
        {
            return "Neat.";
        }

        public function getViewer($action)
        {
            return SSViewer::fromString(
                '<!DOCTYPE html>' . PHP_EOL
                . '<html><head><title>Form upload test</title></head><body>$Form $FormPass</body></html>'
            );
        }
    }
}

<?php

use SilverStripe\Control\HTTPRequest;
use SilverStripe\Dev\FunctionalTest;
use SilverStripe\Dev\SapphireTest;

class PageControllerTest extends FunctionalTest
{
    protected $usesDatabase = true;

    protected function setUp()
    {
        parent::setUp();
        $page = Page::create();
        $page->update(['URLSegment' => 'home'])->write();
        $page->doPublish();
    }

    /**
     * @dataProvider getPassFail
     */
    public function testForm($formName)
    {
        $this->get('/');
        $response = $this->submitForm("Form_$formName", 'action_formSubmission', ['Breaks' => 'two']);
        $this->assertEquals('Neat.', $response->getBody());
    }

    public function getPassFail()
    {
        return [
            ['FormPass'],
            ['Form'],
        ];
    }
}

output

PHPUnit 5.7.27 by Sebastian Bergmann and contributors.

.F                                                                  2 / 2 (100%)

Time: 3.25 seconds, Memory: 44.50MB

There was 1 failure:

1) PageControllerTest::testForm with data set #1 ('Form')
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'Neat.'
+'<!DOCTYPE html>
+<html><head><title>Form upload test</title></head><body>

[...]

FAILURES!
Tests: 2, Assertions: 2, Failures: 1.

Presence of an input type=file causes SimpleTest to (correctly) encode a submission as multi part form data, as opposed to a simple GET style URL Query string.

TestSession though is set up to then run this through parse_str which is designed for query string parsing only.

https://github.com/silverstripe/silverstripe-framework/blob/d408a4e714d4953df9f8552f2f3536223b799b81/src/Dev/TestSession.php#L238

I.e. this part of testing designed specifically for POST vars can only handle GET vars.

Without a file field the submission data becomes Breaks=two&action_formSubmission=Submit With a file field the submission data becomes:

--st5f0ba947b4fc5
Content-Disposition: form-data; name="Breaks"

two
--st5f0ba947b4fc5
Content-Disposition: form-data; name="MAX_FILE_SIZE"

2097152
--st5f0ba947b4fc5
Content-Disposition: form-data; name="action_formSubmission"

Submit
--st5f0ba947b4fc5--

parse_str decodes this into:

array(1) {
  '--st5f0ba947b4fc5
Content-Disposition:_form-data;_name' =>
  string(213) "[... rest of the POST body]

Note:

array size 1
spaces turned to underscores in array key
the value of the only key is the entire remainder of the submission body

https://www.php.net/manual/en/function.parse-str.php#refsect1-function.parse-str-examples

Because variables in PHP can't have dots and spaces in their names, those are converted to underscores. Same applies to naming of respective key names in case of using this function with result parameter.

This effectively wipes all submitted data clean, thus causing a dropdown to have an empty submission (which is invalid). This would in theory (I've not tested this) also mean any required field would also fail if a validator was applied - or in the very least would cause an assertion that something happens with the submitted data to fail, in that there is no submitted data.

There does not appear to be an easy way out of this.

However, this third party library designed for use with PHP4 that has been folded into framework due to the lack of composer at the time (when Silverstripe shifted from SVN to git) is actually actively maintained and is compatible with PHP7 - the "folded in" thirdparty code should probably be dropped from framework in favour of the composer dependency. This may solve the issue. It may not. This is more work than I can spend investigating now. https://github.com/simpletest/simpletest/

After a bit of reading, the best I can find to deal with this is this gist from the comments of this StackOverflow post. Then we would just need a way to work out whether to call parse_str or parse_raw_http_request

Gist content to save you a click

<?php

// The code is inspired by the following discussions and post:
// http://stackoverflow.com/questions/5483851/manually-parse-raw-http-data-with-php/5488449#5488449
// http://www.chlab.ch/blog/archives/webdevelopment/manually-parse-raw-http-data-php

/**
 * Parse raw HTTP request data
 *
 * Pass in $a_data as an array. This is done by reference to avoid copying
 * the data around too much.
 *
 * Any files found in the request will be added by their field name to the
 * $data['files'] array.
 *
 * @param   array  Empty array to fill with data
 * @return  array  Associative array of request data
 */
function parse_raw_http_request($a_data = [])
{
    // read incoming data
    $input = file_get_contents('php://input');

    // grab multipart boundary from content type header
    preg_match('/boundary=(.*)$/', $_SERVER['CONTENT_TYPE'], $matches);

    // content type is probably regular form-encoded
    if (!count($matches))
    {
        // we expect regular puts to containt a query string containing data
        parse_str(urldecode($input), $a_data);
        return $a_data;
    }

    $boundary = $matches[1];

    // split content by boundary and get rid of last -- element
    $a_blocks = preg_split("/-+$boundary/", $input);
    array_pop($a_blocks);

    $keyValueStr = '';
    // loop data blocks
    foreach ($a_blocks as $id => $block)
    {
        if (empty($block))
            continue;

        // you'll have to var_dump $block to understand this and maybe replace \n or \r with a visibile char

        // parse uploaded files
        if (strpos($block, 'application/octet-stream') !== FALSE)
        {
            // match "name", then everything after "stream" (optional) except for prepending newlines
            preg_match("/name=\"([^\"]*)\".*stream[\n|\r]+([^\n\r].*)?$/s", $block, $matches);
            $a_data['files'][$matches[1]] = $matches[2];
        }
        // parse all other fields
        else
        {
            // match "name" and optional value in between newline sequences
            preg_match('/name=\"([^\"]*)\"[\n|\r]+([^\n\r].*)?\r$/s', $block, $matches);
            $keyValueStr .= $matches[1]."=".$matches[2]."&";
        }
    }
    $keyValueArr = [];
    parse_str($keyValueStr, $keyValueArr);
    return array_merge($a_data, $keyValueArr);
}

silverstripe / silverstripe-framework

Presence of FileField breaks FunctionalTest::submitForm #9585

Description

Steps to Reproduce