stavro / arc_ecto

An integration with Arc and Ecto.
255 stars 149 forks source link

Files saved with double extension #39

Open joshuaswilcox opened 8 years ago

joshuaswilcox commented 8 years ago

when using a custom filename that include the actual name of the uploaded file, the extension is duplicated. Here is my code:

defmodule Scotchy.Image do
  use Arc.Definition
  use Arc.Ecto.Definition

  @versions [:original, :thumb]

  def __storage, do: Arc.Storage.Local

  def filename(version,  {file, _scope}), do: "#{version}_#{file.file_name}"

  # Whitelist file extensions:
  def validate({file, _}) do
    ~w(.jpg .jpeg .gif .png .bmp .JPG) |> Enum.member?(Path.extname(file.file_name))
  end

  def storage_dir(_version, {_file, scope}) do
    "uploads/bottle_images/images/#{scope.id}"
  end

  # Define a thumbnail transformation:
  def transform(:thumb, _) do
    {:convert, "-strip -thumbnail 350 -gravity center -extent 350"}
  end

  def default_url(:thumb) do
    "https://placehold.it/350x400"
  end
end

which results in images named thumb_IMG_055.jpg.jpg. I know I can String.replace(file.file_name, ".jpg", "") but that seems hacky

t56k commented 8 years ago

This happens to me too. I lost the custom filename as a quick fix, but obvs that's not ideal.

davidkuhta commented 8 years ago

@joshuaswilcox Try:

def filename(version, {file, _scope}), do: "#{version}_#{Path.basename(file.file_name, Path.extname(file.file_name))}"

See: Arc Local Tests

stavro commented 8 years ago

Ah - Thanks @davidkuhta.

As I currently have it, the "filename" should not include the extension, because different versions of a file may have different extensions. (i.e. you may want the original jpg file stored, but a thumbnail png).

If you have any suggestions on how to make that more clear I'm all 👂

davidkuhta commented 8 years ago

Hi @stavro If I trace the code right I think this is actually more of an Arc consideration. I believe the confusion stems from some examples in the Arc docs, that don't necessarily allude when you override the filename/2 function in storage.ex, you lose the default extension stripping: def filename(_, {file, _}), do: Path.basename(file.file_name, Path.extname(file.file_name))

Particularly Local Configuration and File Names that show: def filename(version, {file, scope}), do: "#{version}-#{file.file_name}"

So my thought would be to either:

  1. Provide a comment in the Arc docs that communicates file_name != basename (and includes Path.basename(file.file_name, Path.extname(file.file_name)))
  2. (My preference) Expose a helper function (say filename/1 in Arc.Definition.Storage) that strips the extension (ultimately what the default filename/2 was doing). So in storage.ex you'd now have:
def filename(_, {file, _}), do: filename(file)
def filename(file), do: Path.basename(file.file_name, Path.extname(file.file_name))

Option 2. would mean you could do things like: def filename(version, {file, _scope}), do: "#{version}_#{filename(file)}" It also wouldn't break anything because the filename/2 still provides the same implementation as it did before.

Note: I chose filename because basename/1 is used by Path but there's nothing to say it couldn't just be that. (Which IMHO is what I would've assumed Path.basename/1 did...) (stemname/1 was another thought)

Anyhow, let me know if either fits your vision for arc/arc_ecto and I'll be happy to send over a PR.