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

Eliminate Kernel::System::FileTemp #545

Closed
bschmalhofer opened this issue Oct 14, 2020 · 3 comments
Closed

Eliminate Kernel::System::FileTemp #545

bschmalhofer opened this issue Oct 14, 2020 · 3 comments
Assignees
Labels
question Further information is requested
Milestone

Comments

@bschmalhofer
Copy link
Contributor

I see no benefits of this module over using File::Temp directly. It does set up a non-standard location for temporary dirs. But I don't think that this is a benefit.

@bschmalhofer bschmalhofer added the enhancement New feature or request label Oct 14, 2020
@bschmalhofer bschmalhofer added this to the Wishlist milestone Oct 14, 2020
@svenoe
Copy link
Contributor

svenoe commented Oct 26, 2020

I don't know every way otobo is set up on systems. If someone wants to give minimal access to the otobo-user, or just have all otobo-files in one place for a better overview, or maybe disk space limitations, it's not a bad thing to be able to, in my opinion.

@bschmalhofer bschmalhofer changed the title Elimninate Kernel::System::FileTemp Eliminate Kernel::System::FileTemp Oct 31, 2020
@bschmalhofer bschmalhofer self-assigned this Dec 4, 2021
@bschmalhofer bschmalhofer added question Further information is requested and removed enhancement New feature or request labels Dec 4, 2021
@bschmalhofer
Copy link
Contributor Author

bschmalhofer commented Dec 4, 2021

Reading the documentation of File::Temp I find that this is trickier than expected, and I agree that keeping Kernel::System::FileTemp might be useful.
However, looking at the code in Kernel/Modules/AgentTicketAttachment.pm, I still find that the object oriented interface of File::Temp would be the better approach. My impression is that Kernel::System::FileTemp first makes an effort to make the created file handle persistent, and then makes an effort to clean up after the end of the current request. File::Temp::new() does kind of the same thing, but makes it easy to limit the life time of the temporary file to a small scope.

Thomas Fahle tells it like it is in https://perl-howto.github.io/2008/07/temporare-dateien-sicher-erzeugen.html:

File::Temp ist seit Perl 5.6.1 ein Standardmodul, d.h. es wird mit Perl ausgeliefert und läuft auf jeder Plattform, die von Perl unterstützt wird.

Dokumentation und API werden selbst von sehr gutmütigen Menschen bestenfalls als verwirrend bezeichnet.

So I propose to keep this issue open, until we find the best solution.

bschmalhofer added a commit that referenced this issue Dec 4, 2021
enhance code comments
more meaningful variable names
bschmalhofer added a commit that referenced this issue Dec 4, 2021
…FileTemp

Issue #545: use the already open file handle
@bschmalhofer
Copy link
Contributor Author

There was an inconstent usage of the file handle returned from Kernel::System::FileTemp::TempFile(). The applied PR aligns the usage in Kernel/Modules/AgentTicketAttachments.pm with the saner usage Kernel/System/Crypt/PGP.pm, A test with the formatter wc -l gave the expected result.

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

No branches or pull requests

2 participants