radarphp / Radar.Adr

The Action-Domain-Responder core for Radar.
MIT License
55 stars 7 forks source link

Default `Input` leaks UI into domain #34

Open jakejohns opened 7 years ago

jakejohns commented 7 years ago

The default Input class does this:

return [
    array_replace(
        (array) $request->getQueryParams(),
        (array) $request->getParsedBody(),
        (array) $request->getUploadedFiles(), // my issue is here
        (array) $request->getCookieParams(),
        (array) $request->getAttributes()
    )
];

I think there's a problem here because $request->getUploadedFiles() will return an array of UploadedFileInterface and it does not seem correct to be relying-on/using/type-hinting/etc such a class inside my domain. The PSR7 interfaces are for HTTP and thereby define the UI. I would think that the Input class is the boundary over which such details do not pass.

It would be silly to write a CLI interface for my domain and require psr/http-message so that when a file is passed at the commandline I can convert it to an UploadedFileInterface.

Here's what I basically ended up doing recently (abstracted a bit from my actual implementation):

<?php
// @codingStandardsIgnoreFile

namespace My\Web\Action\Update;

use Psr\Http\Message\UploadedFileInterface as UploadedFile;
use Psr\Http\Message\ServerRequestInterface as Request;
use SplFileInfo;
use RuntimeException;

/**
 * Dont let PSR7 cross the domain boundary
 */
class Input
{
    // per request tmp dir
    protected $tmpdir;

    // Convert request to domain input
    public function __invoke(Request $request)
    {
        $entity_id = $request->getAttribute('entity_id');
        $data      = $request->getParsedBody();
        $files     = $this->getFiles($request);

        unset($data['submit']);

        return [
            'entity_id' => $entity_id,
            'data'      => $data,
            'files'     => $files
        ];
    }

    // Get an array of SplFileInfo based on uploaded files
    protected function getFiles(Request $request)
    {
        $files    = [];
        $uploaded = $request->getUploadedFiles();

        foreach ($uploaded as $key => $upload) {
            $files[$key] = $this->getFile($upload);
        }

        return $files;
    }

    // Convert an UploadedFileInterface to an SplFileInfo
    protected function getFile(UploadedFile $upload)
    {
        try {
            $tmpdir      = $this->tmpdir();
            $filename    = $upload->getClientFilename();
            $destination = $tmpdir . '/' . $filename;
            $upload->moveTo($destination);

            return new SplFileInfo($destination);

        } catch (RuntimeException $e) {
            return null;
        }
    }

    // Create a temp directory to store all the uploads from this request
    protected function tmpdir()
    {

        if (! $this->tmpdir) {
            $tmpdir = tempnam(sys_get_temp_dir(), 'upload-');

            if (file_exists($tmpdir)) {
                unlink($tmpdir);
            }

            mkdir($tmpdir);
            $this->tmpdir = $tmpdir;
        }

        return $this->tmpdir;
    }
}

I know the default Input here is just a "starting point", but I question if its wise to encourage leaking Http\Message\* into the domain. On the other hand, now I'm hitting the file system in my Input handler, but somehow this seems less wrong to me?

Anyone have any better ideas on this?

pmjones commented 7 years ago

Ugh -- you're right.

Is there perhaps some way to cast the UploadedFile objects back to arrays, or even StdClass objects, so that they match $_FILES more closely? Then at least you don't have to typehint (though you do need to know the structure of the array or object).

cxj commented 4 years ago

What is the domain expecting as input in a case like this? Isn't the whole notion of UploadFile a very HTTP (and $_FILES) oriented concept? Is the domain expecting a file path for a file it can open and manipulate? Something else? It seems to me that maybe the Input handler is the right place to do the conversion -- although avoiding any exception throwing operations might be difficult in some cases.