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

make gaufrette's metadata work #556

Closed
wants to merge 1 commit into from
Closed

make gaufrette's metadata work #556

wants to merge 1 commit into from

Conversation

dswbx
Copy link

@dswbx dswbx commented Apr 11, 2016

make gaufrette's metadata work by creating Gaufrette\File and set metadata there

make gaufrette's metadata work by creating Gaufrette\File and set metadata there
@garak
Copy link
Collaborator

garak commented Dec 17, 2016

@rssfeed can you tell something more about this PR?

@dswbx
Copy link
Author

dswbx commented Dec 17, 2016

It's been a long time, but as I remember, without this changes, files stored via gaufrette in amazon s3 won't get the correct metadata. E.g. a jpg got mime-type (as far as I remember) "application/octet-stream" instead of "image/jpeg". Without having the correct mime-type the browser won't behave as expected.

With my pull request changes in place the metadata is being added correctly.

@Koc
Copy link
Contributor

Koc commented Dec 20, 2016

@rssfeed can you add test that confirm fix and prevent regression in future?

@garak
Copy link
Collaborator

garak commented Dec 22, 2016

@rssfeed did you take a look to #163?

@OJezu
Copy link

OJezu commented Nov 1, 2017

What is preventing this PR from being merged?

@garak
Copy link
Collaborator

garak commented Nov 2, 2017

  • This branch has conflicts that must be resolved
  • Missing tests
  • Awaiting feedback about another possible related issue

@OJezu
Copy link

OJezu commented Nov 2, 2017

This does not fix #163, as when setting metadata in Gaufrette File, Gaufrette first tries to save metadata, and then the file itself. See KnpLabs/Gaufrette#537

Upon closer inspection I don't know if this PR actually changes anything, as

$gaufretteFile = new File($path, $filesystem);
$gaufretteFile->setContent(file_get_contents($file->getPathname()), array('contentType' => $file->getMimeType()));

Underneath calls the same $filesystem->getAdapter()->setMetadata and $filesystem->getAdapter()->write. Workaround for Gaufrette issues would be to write file first, and then setMetadata.

If KnpLabs/Gaufrette#537 is fixed, then

$gaufretteFile = new File($path, $filesystem);
$gaufretteFile->setContent(file_get_contents($file->getPathname()), array('contentType' => $file->getMimeType());

Could be done no matter if adapter supports metadata or not, and I guess that would be the best solution. It should be up to Gaufrette to save metadata along with file in most atomic way.

@garak
Copy link
Collaborator

garak commented Nov 2, 2017

So, what we can do here is waiting for Gaufrette feedback, then close this PR (that looks too old) and open a new one.

@OJezu
Copy link

OJezu commented Nov 2, 2017

Without Gaufrette feedback, Uploader can save file first, then metadata at risk of file being saved without metadata. Or retry saving metadata after file upload, if it failed before. Those are options to consider, if Gaufrette is not fixed.

I looked into fixing Gaufrette myself, but after seeing code I put it off - there is no way to do it there without risking some (internal) BC changes. I will see how KnpLabs/Gaufrette#537 goes.

@garak
Copy link
Collaborator

garak commented Jul 21, 2022

Closing because it's too old and without any feedback

@garak garak closed this Jul 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants