Yii2: MultipartFormDataParser returns incorrect decoded data

[PowerGamer1]

What steps will reproduce the problem?

// Assume we are handling some POST request.
$encodedData = file_get_contents('some-local-server-file-NOT-a-data-from-POST-request.txt');
$data = MultipartFormDataParser::parse($encodedData);

What is the expected result?

$data corresponds to $encodedData.

What do you get instead?

$data corresponds to whatever client has passed in the HTTP POST request body.

Code to blame:

    public function parse($rawBody, $contentType)
    {
        if (!empty($_POST) || !empty($_FILES)) {
            // normal POST request is parsed by PHP automatically
            return $_POST;
        }

[klimov-paul]

Resolved by commit 578b2caf424417215309a827a127dac1394e53ac

[PowerGamer1]

This is just a bad design IMO. While I understand the need for performance boost, any sane person looking at the interface of RequestParserInterface would (rightfully) expect parse function to actually parse $rawBody and not something else entirely. You basically break your own interface in the MultipartFormDataParser class.

interface RequestParserInterface
{
    public function parse($rawBody, $contentType);
}

I think it would have been a much better solution to do a check if PHP has already parsed data into $_POST _outside_ the MultipartFormDataParser::parse() function - in a place were this function is called (I think currently its a single place - Request::getBodyParams()). So if a PHP already parsed the data return $_POST otherwise call MultipartFormDataParser::parse().

I urge you to rollback your "fix", reopen this issue and consider this alternative solution. At the very least do it properly in 2.1.

[PowerGamer1]

Also, I must add, the fact that MultipartFormDataParser::parse() modifies PHP's superglobals like $_POST and $_FILES is also a bad design IMO. A question arises, why other parsers do not (like JsonParser) but this one does? Either ALL parsers should modify those superglobals or NONE of them should. The latter one would be the best (you can't make PHP treat the files you forcefully put into $_FILES as a true uploaded files anyway, so just return them separately).

This is another design problem that should be fixed in 2.1.

[klimov-paul]

MultipartFormDataParser does not follow RequestParserInterface purely anyway, since it modifies superglobal $_FILES in process. To make the real proper fix, interface should be changed in order to accept the Request object instead of strings, and request should be changed to store uploaded files internally.
These are tracked by separated issues.

[PowerGamer1]

@klimov-paul
Those are the thoughts in the right direction, however I am unable to find any issues mentioning a redesign of MultipartFormDataParser or RequestParserInterface (either open or closed). It would be nice if you find them and link this issue into them.

[klimov-paul]

11328

10824

14339

While being resolved these issues will cause parser and formatter changed as well.