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

Added uniqid() filename option #63

Closed
wants to merge 3 commits into from
Closed

Added uniqid() filename option #63

wants to merge 3 commits into from

Conversation

lucavallin
Copy link

In a project of mine I needed to save attachments using a unique filename.
Now you can choose between the original filename or a unique one by simply setting $use_uniqueFilename = true when calling saveAttachments($attach_dir, $use_uniqueFilename = false).
Unique filename is generated using uniqid().

@eXorus
Copy link
Member

eXorus commented Oct 1, 2015

Thanks, could you check code coverage and continuous integration before I can merge it ?

@lucavallin
Copy link
Author

Surely I will, probably in the weekend.

@eXorus
Copy link
Member

eXorus commented Nov 12, 2015

The weekend was very long :)

@lucavallin
Copy link
Author

lucavallin commented Nov 12, 2015

I'm sorry, I moved to another country in the meantime and I forgot about the PR. I doubt I will have the room to focus on this any time soon.

@necenzurat
Copy link

Is this week on Pluto?

@eXorus
Copy link
Member

eXorus commented Mar 20, 2016

Someone to continue ? :)

@gmta
Copy link
Contributor

gmta commented Aug 17, 2016

This change feels awkward to me for a couple of reasons:

  • It generates unique filenames for attachments whether you use them or not
  • It uses uniqid() instead of tempnam() which was basically made for generating unique filenames in a specific directory
  • It expands an API method with a boolean argument which is not Best Practice TM
  • It changes the public Attachment API (constructor argument)

However, because email attachments can have filename collisions, this is a needed feature in some form. It also exposes a bug in the current Parser->saveAttachments() method: it overwrites attachments with the same name.

I would suggest implementing it as follows:

  • Add a test to expose the ->saveAttachments() issue with duplicate filenames
  • Fix ->saveAttachments() by adding a behavioral argument:
    1. Use original filenames and throw exception on duplicates (default)
    2. Use original filenames but generate unique suffix on duplicates
    3. Always generate fully unique filenames

@eXorus what do you think?

@eXorus
Copy link
Member

eXorus commented Feb 14, 2017

Sorry I didn't saw your contribution @gmta

I think it's a very good proposition I will prefer to have the option 2 by default. And I agree with all your remarks.

Thanks, could you do this PR ?

@gmta
Copy link
Contributor

gmta commented Feb 20, 2017

@eXorus Sure, I've just created PR #139.

@eXorus eXorus closed this Feb 28, 2017
@amrkamboj1988
Copy link

amrkamboj1988 commented Oct 29, 2017

It's dropping the extension of the attachment. How to make sure to generate a file with relevant exptension in the case of duplicity.

@gmta
Copy link
Contributor

gmta commented Oct 29, 2017

Hi @amrkamboj1988, you should probably raise a new issue for that. The proposed solution here does indeed drop the filename extension, but you could create a PR since adding the extension back is relatively simple. However, please also consider not using file extensions but rather Content-Type for file type detection.

@lucavallin lucavallin changed the title Added uniqid() filename possibility Added uniqid() filename option Sep 7, 2023
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.

None yet

5 participants