8000 [Filesystem] Add $suffix argument to tempnam() by jdufresne · Pull Request #33003 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Filesystem] Add $suffix argument to tempnam() #33003

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Feb 4, 2020
Merged

[Filesystem] Add $suffix argument to tempnam() #33003

merged 1 commit into from
Feb 4, 2020

Conversation

jdufresne
Copy link
Contributor
@jdufresne jdufresne commented Aug 7, 2019 •
edited
Loading
Q A
Branch? 4.4
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #33002
License MIT
Doc PR symfony/symfony-docs#12108

Description

The tempnam() interface was previously:

tempnam($dir, $prefix)

This adds a third argument, $suffix, that is appended to the filename after the random component. This defaults to '' for backwards compatibility. This is quite useful when the temporary file is consumed for a specific purpose that expects a suffix.

Example

$filesystem->tempnam('/tmp', 'prefix_', '.png');

Would create a file like: /tmp/prefix_abcd1234.png.

derrabus
derrabus requested changes Aug 7, 2019
View reviewed changes
@nicolas-grekas nicolas-grekas added this to the next milestone Aug 7, 2019
@jdufresne
Copy link
Contributor Author

Thanks for the reviews. I have made the suggested changes.

@derrabus
Copy link
Member
derrabus commented Aug 7, 2019

The test failures on Travis seem to be related to your changes.

@jdufresne
Copy link
Contributor Author

Thanks. I fixed my code/tests so they are passing again. Travis is now failing for reasons that appear to be unrelated to this PR.

@yceruto
Copy link
Member
yceruto commented Aug 7, 2019

@jdufresne is your current branch up to date with 4.4?

@jdufresne
Copy link
Contributor Author
jdufresne commented Aug 7, 2019 •
edited
Loading

The parent commit is d3a7be8 (a few new commits landed minutes ago).

Looks like the 4.4 branch is also failing with the same errors as of today.

@derrabus
Copy link
Member
derrabus commented Aug 7, 2019

The test failures have been caused by Twig. The issue should be fixed with twigphp/Twig@debc5c3

@yceruto yceruto mentioned this pull request Aug 7, 2019
Closed
@jdufresne
Copy link
Contributor Author

Thanks for the review! I've followed the linked example by dropping the argument and using the shim.

@javiereguiluz
Copy link
Member

I'm divided about this proposal. The prefix is needed because this method returns an absolute path to a file, so it's not trivial to prefix only the file name.

But, adding a suffix should always be simple because it's safe to append a string to the value returned by this function. So, I'd probably close this as "won't fix".

However, another option would be to change the current "prefix" option to be a "pattern" option with only one placeholder called {filename}, so you can do this:

// Before
$fs->tempnam('/tmp', 'prefix_');
$fs->tempnam('/tmp', 'prefix_', '.png');

// After
$fs->tempnam('/tmp', 'prefix_{filename}');
$fs->tempnam('/tmp', 'prefix_{filename}.png');

@pierredup
Copy link
Contributor

The prefix is needed because this method returns an absolute path to a file, so it's not trivial to prefix only the file name.

The prefix is actually a required parameter in the native tempnam function which this method uses, so it's not just about the path.

adding a suffix should always be simple because it's safe to append a string to the value returned by this function

The problem here is that you will then manually need to create the file and remove the original file (as the method creates the file as well and doesn't just return a path).

So going from

$file = $fs->tempnam('/tmp', 'prefix_', '.png');

to this

$file = $fs->tempnam('/tmp', 'prefix_');
$fs->remove($file); // remove created file
$file .= '.png';
$fs->touch($file); // create new file

@javiereguiluz
Copy link
Member

@pierredup you explained it well. I think I'm now a bit more 👍 about this proposal.

@jdufresne
Copy link
Contributor Author

I've made the suggested change from @stof and dropped the deprecation warning so that DebugClassLoader handles it.

@jdufresne
Copy link
Contributor Author

Thanks. I understand now. I have applied the suggested patch in the latest revision.

@jdufresne
Copy link
Contributor Author

fabbot is suggesting that I should remove $suffix from the docblock. Is this expected? Should I remove it? Or should I alter it a specific way to appease the bot?

@OskarStark
Copy link
Contributor

Should I remove it? Or should I alter it a specific way to appease the bot?

I would say its a false/positive

@jdufresne
Copy link
Contributor Author

Thanks. At this stage is there anything I should do to move this PR forward?

@fabpot fabpot changed the base branch from 4.4 to master February 4, 2020 08:10
@fabpot
Copy link
Member
fabpot commented Feb 4, 2020

Thank you @jdufresne.

@fabpot fabpot closed this in 7784d9f Feb 4, 2020
@fabpot fabpot merged commit ef12069 into symfony:master Feb 4, 2020
@nicolas-grekas nicolas-grekas modified the milestones: next, 5.1 May 4, 2020
@fabpot fabpot mentioned this pull request May 5, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stof stof stof left review comments

@pierredup pierredup pierredup requested changes

@derrabus derrabus derrabus requested changes

@yceruto yceruto yceruto requested changes

@fabpot fabpot fabpot approved these changes

Assignees
No one assigned
Labels
Feature Filesystem Status: Reviewed
Projects
None yet
Milestone
5.1
Development

Successfully merging this pull request may close these issues.
10 participants
@jdufresne @derrabus @yceruto @javiereguiluz @pierredup @OskarStark @fabpot @stof @nicolas-grekas @carsonbot
0