Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Default Input leaks UI into domain #34

Open
jakejohns opened this issue Sep 20, 2017 · 2 comments
Open

Default Input leaks UI into domain #34

jakejohns opened this issue Sep 20, 2017 · 2 comments

Comments

@jakejohns
Copy link
Contributor

jakejohns commented Sep 20, 2017

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
Copy link

pmjones commented Sep 20, 2017

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
Copy link
Contributor

cxj commented Nov 28, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants