FriendsOfCake / cakephp-upload

CakePHP: Handle file uploading sans ridiculous automagic
https://cakephp-upload.readthedocs.io/
MIT License
550 stars 255 forks source link

Setting a config with ContentType during write operations #511

Open challgren opened 5 years ago

challgren commented 5 years ago

So for Rackspace and Amazon S3 a ContentType/Content-Type is required when uploading a file, as the service doesn't automatically set a ContentType for objects that are uploaded. If the content type is not set correctly the uploaded file will not render in the browser correctly when the end user views the uploaded file.

Example upload a PDF and then view the Amazon S3 link. The default content type will be set to text/plain when it should be set to application/pdf in Chrome this would cause the PDF to not render with an error of unknown file type. On Rackspace Chrome would attempt to download the file instead of displaying as a application/pdf, as I no longer use Rackspace I am unable to provide what really happens.

The problem comes into play because we are using streams to write files and the S3 adapter does attempt to set the ContentType but because the supplied data is a stream its unable to call https://github.com/thephpleague/flysystem/blob/master/src/Util/MimeType.php#L182 and use finfo. I could do the below code again for S3 but I do know that Amazon S3 charges based on REQUESTs so this could become an "expensive" workaround.

When using the Rackspace Adapter I would have to

public function afterSave(Event $event, EntityInterface $entity, ArrayObject $options)
    {
        if ($entity->isDirty('photo') && $this->adapter) {
            try {
                $obj = $this->adapter->getContainer()->getObject($entity->get('photo_dir') . $entity->get('photo'));
                $obj->saveMetadata(['Content-Type' => $options['content-type']], false);
            } catch (\Exception $exception) {
            }
        }
    }

to correct the content type. The Rackspace file adapter makes no attempt to automatically set the ContentType.

I did open an Issue with AWS S3 Flysystem https://github.com/thephpleague/flysystem-aws-s3-v3/issues/167 but they suggested manually setting the Config when writing the file. However the current implementations prevent that from being possible.

One possible solution is to write a CustomWriter for Amazon and Rackspace, or modify the DefaultWriter to finfo before the file is converted into a stream and supply the ContentType and Content-Type to the Config param when calling $flysystem->writeStream(). I do think that by including these additional Writers developers may assume that they don't need to configure an Flysystem Adapter. I do foresee many other developers having the same issues.

I'm open to discussion for the best possible solution.

ADmad commented 5 years ago

The $data property of writer instance already contains the mime type under type key. The DefaultWriter::writeFile() can be modified to use that value and set mime type for flysystem.

challgren commented 5 years ago

But it's different for each provider. Aws is ContentType and rackspace is Content-Type

ADmad commented 5 years ago

Doh, too bad flysystem doesn't abstract that, that's the whole point of using that lib :slightly_smiling_face:.

challgren commented 5 years ago

I can modify it and just blindly send both vars in the config if you feel that is an acceptable solution.

ADmad commented 5 years ago

I think think the flysystem adapter should be able to infer the type itself and not require setting it explicitly.

challgren commented 5 years ago

I agree, but really it's the damn cloud providers. Aws attempts to correct it but rackspace doesn't even try. I'll make a PR and see what y'all think.

challgren commented 5 years ago

What would be better, not using a stream maybe? Is there a reason why a stream is forced?

Not using a stream would solve the AWS Adapter ContentType but not Rackspace Adapter.

I could submit an Issue for the Rackspace Adapter for not inering the Content-Type

challgren commented 5 years ago

So my solution for Amazon S3 was to extend the default writer like below.

<?php
namespace App\File\Writer;

use Josegonzalez\Upload\File\Writer\DefaultWriter;
use League\Flysystem\FilesystemInterface;

class AppWriter extends DefaultWriter
{
    public function writeFile(FilesystemInterface $filesystem, $file, $path)
    {
        $stream = @fopen($file, 'r');
        $config = ['ContentType' => $this->data['type']];
        if ($stream === false) {
            return false;
        }

        $success = false;
        $tempPath = $path . '.temp';
        $this->deletePath($filesystem, $tempPath);
        if ($filesystem->writeStream($tempPath, $stream, $config)) {
            $this->deletePath($filesystem, $path);
            $success = $filesystem->rename($tempPath, $path);
        }
        $this->deletePath($filesystem, $tempPath);
        is_resource($stream) && fclose($stream);

        return $success;
    }
}
challgren commented 5 years ago

The solution for Rackspace (untested) is

<?php
namespace App\File\Writer;

use Josegonzalez\Upload\File\Writer\DefaultWriter;
use League\Flysystem\FilesystemInterface;

class AppWriter extends DefaultWriter
{
    public function writeFile(FilesystemInterface $filesystem, $file, $path)
    {
        $stream = @fopen($file, 'r');
        $config = ['headers' => ['Content-Type' => $this->data['type']]];
        if ($stream === false) {
            return false;
        }

        $success = false;
        $tempPath = $path . '.temp';
        $this->deletePath($filesystem, $tempPath);
        if ($filesystem->writeStream($tempPath, $stream, $config)) {
            $this->deletePath($filesystem, $path);
            $success = $filesystem->rename($tempPath, $path);
        }
        $this->deletePath($filesystem, $tempPath);
        is_resource($stream) && fclose($stream);

        return $success;
    }
}
challgren commented 5 years ago

Related #402

jabberdabber commented 5 years ago

@challgren Thank you for posting your code. It worked with my Rackspace CDN.

josegonzalez commented 5 years ago

Might be something we can upstream the fix for @challgren ?

cpierce commented 2 years ago

newest code is:

namespace App\File\Writer;

use Josegonzalez\Upload\File\Writer\DefaultWriter;
use League\Flysystem\FilesystemOperator;
use League\Flysystem\FilesystemException;

/**
 * App Writer Class
 * 
 */
class AppWriter extends DefaultWriter
{

    /**
     * Writes a set of files to an output
     *
     * @param \League\Flysystem\FilesystemOperator $filesystem a filesystem wrapper
     * @param string $file a full path to a temp file
     * @param string $path that path to which the file should be written
     * @return bool
     */
    public function writeFile(FilesystemOperator $filesystem, $file, $path): bool
    {
        $stream = @fopen($file, 'r');
        if ($stream === false) {
            return false;
        }

        $config = [
            'ContentType' => $this->data->getClientMediaType(),
        ];

        $success = false;
        $tempPath = $path . '.temp';
        $this->deletePath($filesystem, $tempPath);
        try {
            $filesystem->writeStream($tempPath, $stream, $config);
            $this->deletePath($filesystem, $path);
            try {
                $filesystem->move($tempPath, $path);
                $success = true;
            } catch (FilesystemException $e) {
                // noop
            }
        } catch (FilesystemException $e) {
            // noop
        }
        $this->deletePath($filesystem, $tempPath);
        is_resource($stream) && fclose($stream);

        return $success;
    }
}

Thanks to @challgren for pointing me in the right direction.