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

Setting a config with ContentType during write operations #511

Open
challgren opened this issue Apr 3, 2019 · 13 comments
Open

Setting a config with ContentType during write operations #511

challgren opened this issue Apr 3, 2019 · 13 comments

Comments

@challgren
Copy link
Contributor

challgren commented Apr 3, 2019

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.

@challgren challgren changed the title Setting a config during write operations Setting a config with ContentType during write operations Apr 3, 2019
@ADmad
Copy link
Member

ADmad commented Apr 3, 2019

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

challgren commented Apr 3, 2019

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

@ADmad
Copy link
Member

ADmad commented Apr 3, 2019

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

@challgren
Copy link
Contributor Author

challgren commented Apr 3, 2019

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

@ADmad
Copy link
Member

ADmad commented Apr 3, 2019

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

@challgren
Copy link
Contributor Author

challgren commented Apr 3, 2019

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

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

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

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

Related #402

@jabberdabber
Copy link

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

@josegonzalez
Copy link
Member

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

@cpierce
Copy link
Contributor

cpierce commented Oct 5, 2022

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.

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

No branches or pull requests

5 participants