Suggestion: Add optional suffix to tempnam()

php.internals

Stefan Neufeind

12 years ago
Hi, I'd like to propose adding an optional $suffix to tempnam(). Please find attached a patch that implements this against current master. Help with this would be appreciated, especially if I did something wrong, since it's my first php-src patch. As you might expect, it can be used like echo tempnam('.', 'abc-', '.png'); We discussed the usecase lately in the TYPO3-community where there are some cases where external tools would like to have a proper suffix for temporary files. They currently use their own "workaround" to have a tempnam() with suffix. Kind regards, Stefan

Yasuo Ohgaki

12 years ago
Hi Stefan, On Sun, Jan 12, 2014 at 4:15 AM, Stefan Neufeind <neufeind@php.net> wrote:
> I'd like to propose adding an optional $suffix to tempnam(). Please find > attached a patch that implements this against current master. >
It's convenient if you could use github instead of attaching patch. Could you use github or gist? Thank you.
-- Yasuo Ohgaki yohgaki@ohgaki.net

Johannes Schlueter

12 years ago
On Sat, 2014-01-11 at 20:15 +0100, Stefan Neufeind wrote:
> -static int php_do_open_temporary_file(const char *path, const char *pfx, char **opened_path_p TSRMLS_DC) > +static int php_do_open_temporary_file(const char *path, const char *pfx, char **opened_path_p TSRMLS_DC, const char *suffix)
TSRM should be the last parameter, move your new one in front of it.
> -PHPAPI int php_open_temporary_fd_ex(const char *dir, const char *pfx, char **opened_path_p, zend_bool open_basedir_check TSRMLS_DC) > +PHPAPI int php_open_temporary_fd_ex(const char *dir, const char *pfx, char **opened_path_p, zend_bool open_basedir_check TSRMLS_DC, const char *suffix)
> -PHPAPI int php_open_temporary_fd(const char *dir, const char *pfx, char **opened_path_p TSRMLS_DC) > +PHPAPI int php_open_temporary_fd(const char *dir, const char *pfx, char **opened_path_p TSRMLS_DC, const char *suffix)
> -PHPAPI FILE *php_open_temporary_file(const char *dir, const char *pfx, char **opened_path_p TSRMLS_DC) > +PHPAPI FILE *php_open_temporary_file(const char *dir, const char *pfx, char **opened_path_p TSRMLS_DC, const char *suffix)
[...] These are API changes so it could only be in 5.6.0. Eventually it might be added if instead of changing APIs new functions might be introduced and the old ones call the new ones. (also TSRM should be last last there, too) Aside for that: A test would be good. For not loosing it either create a feature request at bugs.php.net or pull request at github. johannes

Stefan Neufeind

12 years ago
On 01/12/2014 09:40 PM, Johannes Schlüter wrote:
> On Sat, 2014-01-11 at 20:15 +0100, Stefan Neufeind wrote: >> -static int php_do_open_temporary_file(const char *path, const char *pfx, char **opened_path_p TSRMLS_DC) >> +static int php_do_open_temporary_file(const char *path, const char *pfx, char **opened_path_p TSRMLS_DC, const char *suffix) > > TSRM should be the last parameter, move your new one in front of it. > >> -PHPAPI int php_open_temporary_fd_ex(const char *dir, const char *pfx, char **opened_path_p, zend_bool open_basedir_check TSRMLS_DC) >> +PHPAPI int php_open_temporary_fd_ex(const char *dir, const char *pfx, char **opened_path_p, zend_bool open_basedir_check TSRMLS_DC, const char *suffix) > >> -PHPAPI int php_open_temporary_fd(const char *dir, const char *pfx, char **opened_path_p TSRMLS_DC) >> +PHPAPI int php_open_temporary_fd(const char *dir, const char *pfx, char **opened_path_p TSRMLS_DC, const char *suffix) > >> -PHPAPI FILE *php_open_temporary_file(const char *dir, const char *pfx, char **opened_path_p TSRMLS_DC) >> +PHPAPI FILE *php_open_temporary_file(const char *dir, const char *pfx, char **opened_path_p TSRMLS_DC, const char *suffix) > [...] > > These are API changes so it could only be in 5.6.0. Eventually it might > be added if instead of changing APIs new functions might be introduced > and the old ones call the new ones. > (also TSRM should be last last there, too) > > Aside for that: A test would be good. > > For not loosing it either create a feature request at bugs.php.net or > pull request at github.
Hi, I've sent a reply to Ferenc and David some days ago. But maybe somebody else could also help get this forward a bit. RFC is here, together with a pull-request: https://wiki.php.net/rfc/tempnam-suffix I also tried creating a testcase. For some reason it fails - but from the commandline I don't see why. I guess it's because my ".png"-example fails to validate against the expected result or something? The actual output of the script for testing is fine for me though. Would be awesome if somebody could pick this up please. Kind regards, Stefan

Christopher Jones

12 years ago
On 01/30/2014 01:11 AM, Stefan Neufeind wrote:
> On 01/12/2014 09:40 PM, Johannes Schlüter wrote: >> On Sat, 2014-01-11 at 20:15 +0100, Stefan Neufeind wrote: >>> -static int php_do_open_temporary_file(const char *path, const char *pfx, char **opened_path_p TSRMLS_DC) >>> +static int php_do_open_temporary_file(const char *path, const char *pfx, char **opened_path_p TSRMLS_DC, const char *suffix) >> >> TSRM should be the last parameter, move your new one in front of it. >> >>> -PHPAPI int php_open_temporary_fd_ex(const char *dir, const char *pfx, char **opened_path_p, zend_bool open_basedir_check TSRMLS_DC) >>> +PHPAPI int php_open_temporary_fd_ex(const char *dir, const char *pfx, char **opened_path_p, zend_bool open_basedir_check TSRMLS_DC, const char *suffix) >> >>> -PHPAPI int php_open_temporary_fd(const char *dir, const char *pfx, char **opened_path_p TSRMLS_DC) >>> +PHPAPI int php_open_temporary_fd(const char *dir, const char *pfx, char **opened_path_p TSRMLS_DC, const char *suffix) >> >>> -PHPAPI FILE *php_open_temporary_file(const char *dir, const char *pfx, char **opened_path_p TSRMLS_DC) >>> +PHPAPI FILE *php_open_temporary_file(const char *dir, const char *pfx, char **opened_path_p TSRMLS_DC, const char *suffix) >> [...] >> >> These are API changes so it could only be in 5.6.0. Eventually it might >> be added if instead of changing APIs new functions might be introduced >> and the old ones call the new ones. >> (also TSRM should be last last there, too) >> >> Aside for that: A test would be good. >> >> For not loosing it either create a feature request at bugs.php.net or >> pull request at github. > > Hi, > > I've sent a reply to Ferenc and David some days ago. But maybe somebody > else could also help get this forward a bit. > > RFC is here, together with a pull-request: > https://wiki.php.net/rfc/tempnam-suffix > > I also tried creating a testcase. For some reason it fails - but from > the commandline I don't see why. I guess it's because my ".png"-example > fails to validate against the expected result or something? > The actual output of the script for testing is fine for me though. > > Would be awesome if somebody could pick this up please. > > > Kind regards, > Stefan >
The RFC needs some basic info, e.g. is there a default for the optional suffix? What are any potential porting issues for users (or for the implementation)? Also, make it easy for readers by giving an example of what happens now and what happens with the new implementation. Having a failing testcase isn't going to help tempt someone to spend time merging the code. Perhaps you can revisit this and get it working? (Or state that if the RFC is accepted, you will do the work to get it working). Are any existing tests broken by the change? Chris
-- christopher.jones@oracle.com http://twitter.com/ghrd Free PHP & Oracle book: http://www.oracle.com/technetwork/topics/php/underground-php-oracle-manual-098250.html

Stefan Neufeind

12 years ago
Hi, On 01/30/2014 07:08 PM, Christopher Jones wrote:
> > On 01/30/2014 01:11 AM, Stefan Neufeind wrote: >> On 01/12/2014 09:40 PM, Johannes Schlüter wrote: >>> On Sat, 2014-01-11 at 20:15 +0100, Stefan Neufeind wrote: >>>> -static int php_do_open_temporary_file(const char *path, const char >>>> *pfx, char **opened_path_p TSRMLS_DC) >>>> +static int php_do_open_temporary_file(const char *path, const char >>>> *pfx, char **opened_path_p TSRMLS_DC, const char *suffix) >>> >>> TSRM should be the last parameter, move your new one in front of it. >>> >>>> -PHPAPI int php_open_temporary_fd_ex(const char *dir, const char >>>> *pfx, char **opened_path_p, zend_bool open_basedir_check TSRMLS_DC) >>>> +PHPAPI int php_open_temporary_fd_ex(const char *dir, const char >>>> *pfx, char **opened_path_p, zend_bool open_basedir_check TSRMLS_DC, >>>> const char *suffix) >>> >>>> -PHPAPI int php_open_temporary_fd(const char *dir, const char *pfx, >>>> char **opened_path_p TSRMLS_DC) >>>> +PHPAPI int php_open_temporary_fd(const char *dir, const char *pfx, >>>> char **opened_path_p TSRMLS_DC, const char *suffix) >>> >>>> -PHPAPI FILE *php_open_temporary_file(const char *dir, const char >>>> *pfx, char **opened_path_p TSRMLS_DC) >>>> +PHPAPI FILE *php_open_temporary_file(const char *dir, const char >>>> *pfx, char **opened_path_p TSRMLS_DC, const char *suffix) >>> [...] >>> >>> These are API changes so it could only be in 5.6.0. Eventually it might >>> be added if instead of changing APIs new functions might be introduced >>> and the old ones call the new ones. >>> (also TSRM should be last last there, too) >>> >>> Aside for that: A test would be good. >>> >>> For not loosing it either create a feature request at bugs.php.net or >>> pull request at github. >> >> Hi, >> >> I've sent a reply to Ferenc and David some days ago. But maybe somebody >> else could also help get this forward a bit. >> >> RFC is here, together with a pull-request: >> https://wiki.php.net/rfc/tempnam-suffix >> >> I also tried creating a testcase. For some reason it fails - but from >> the commandline I don't see why. I guess it's because my ".png"-example >> fails to validate against the expected result or something? >> The actual output of the script for testing is fine for me though. >> >> Would be awesome if somebody could pick this up please. > > The RFC needs some basic info, e.g. is there a default for the > optional suffix? What are any potential porting issues for users > (or for the implementation)? Also, make it easy for readers by giving an > example of what happens now and what happens with the new > implementation.
Well, it's an additional option. If you don't give it, it behaves as before. And as commonly with a suffix: If you don't specify one, you don't get a suffix. Sorry I don't know what is unclear. But maybe it's just an experience-thing how "explicitly" you usually handle such things in an RFC.
> Having a failing testcase isn't going to help tempt someone to spend > time merging the code. Perhaps you can revisit this and get it > working? (Or state that if the RFC is accepted, you will do the work > to get it working). Are any existing tests broken by the change?
I tried to find out, but didn't manage to get a "verbose" output of the testcase-runner on the console. It just said "failed" for me but didn't give me the returned / expected output or so. So it's more a question of "how to get it right". The testcode in the test itself works. I suspect a notation-thing that for somebody experienced will be peanuts. I currently run it with: TESTS=ext/standard/tests/file/tempnam_suffix.phpt make test Is there a better way to do it? Kind regards, Stefan

Christopher Jones

12 years ago
On 01/30/2014 10:55 AM, Stefan Neufeind wrote:
> Hi, > > On 01/30/2014 07:08 PM, Christopher Jones wrote: >> >> On 01/30/2014 01:11 AM, Stefan Neufeind wrote: >>> On 01/12/2014 09:40 PM, Johannes Schlüter wrote: >>>> On Sat, 2014-01-11 at 20:15 +0100, Stefan Neufeind wrote: >>>>> -static int php_do_open_temporary_file(const char *path, const char >>>>> *pfx, char **opened_path_p TSRMLS_DC) >>>>> +static int php_do_open_temporary_file(const char *path, const char >>>>> *pfx, char **opened_path_p TSRMLS_DC, const char *suffix) >>>> >>>> TSRM should be the last parameter, move your new one in front of it. >>>> >>>>> -PHPAPI int php_open_temporary_fd_ex(const char *dir, const char >>>>> *pfx, char **opened_path_p, zend_bool open_basedir_check TSRMLS_DC) >>>>> +PHPAPI int php_open_temporary_fd_ex(const char *dir, const char >>>>> *pfx, char **opened_path_p, zend_bool open_basedir_check TSRMLS_DC, >>>>> const char *suffix) >>>> >>>>> -PHPAPI int php_open_temporary_fd(const char *dir, const char *pfx, >>>>> char **opened_path_p TSRMLS_DC) >>>>> +PHPAPI int php_open_temporary_fd(const char *dir, const char *pfx, >>>>> char **opened_path_p TSRMLS_DC, const char *suffix) >>>> >>>>> -PHPAPI FILE *php_open_temporary_file(const char *dir, const char >>>>> *pfx, char **opened_path_p TSRMLS_DC) >>>>> +PHPAPI FILE *php_open_temporary_file(const char *dir, const char >>>>> *pfx, char **opened_path_p TSRMLS_DC, const char *suffix) >>>> [...] >>>> >>>> These are API changes so it could only be in 5.6.0. Eventually it might >>>> be added if instead of changing APIs new functions might be introduced >>>> and the old ones call the new ones. >>>> (also TSRM should be last last there, too) >>>> >>>> Aside for that: A test would be good. >>>> >>>> For not loosing it either create a feature request at bugs.php.net or >>>> pull request at github. >>> >>> Hi, >>> >>> I've sent a reply to Ferenc and David some days ago. But maybe somebody >>> else could also help get this forward a bit. >>> >>> RFC is here, together with a pull-request: >>> https://wiki.php.net/rfc/tempnam-suffix >>> >>> I also tried creating a testcase. For some reason it fails - but from >>> the commandline I don't see why. I guess it's because my ".png"-example >>> fails to validate against the expected result or something? >>> The actual output of the script for testing is fine for me though. >>> >>> Would be awesome if somebody could pick this up please. >> >> The RFC needs some basic info, e.g. is there a default for the >> optional suffix? What are any potential porting issues for users >> (or for the implementation)? Also, make it easy for readers by giving an >> example of what happens now and what happens with the new >> implementation. > > Well, it's an additional option. If you don't give it, it behaves as > before. And as commonly with a suffix: If you don't specify one, you > don't get a suffix. > > Sorry I don't know what is unclear. But maybe it's just an > experience-thing how "explicitly" you usually handle such things in an RFC.
Prior PHP RFCs are not the best examples of RFCs in the world. Feel free to do better! One rule would be if something has to be explained on the mail list, then it should be in the RFC. Also, from experience, RFCs should state what they do, and also what they don't do. It's better not to leave any gray areas. Don't forget to have content that can be reused for the PHP manual (See #8 in https://blogs.oracle.com/opal/entry/the_mysterious_php_rfc_process).
> >> Having a failing testcase isn't going to help tempt someone to spend >> time merging the code. Perhaps you can revisit this and get it >> working? (Or state that if the RFC is accepted, you will do the work >> to get it working). Are any existing tests broken by the change? > > I tried to find out, but didn't manage to get a "verbose" output of the > testcase-runner on the console. It just said "failed" for me but didn't > give me the returned / expected output or so. So it's more a question of > "how to get it right". The testcode in the test itself works. I suspect > a notation-thing that for somebody experienced will be peanuts.
I'd never take that for granted. It will take some fiddling, I'm sure. I can understand you might not want to spend time on it if there is a possibility the RFC isn't going to be accepted. However I don't see your RFC as something that would fail to pass a vote. Check the .log, .exp files etc from the test run and post questions on the mail list.
> > I currently run it with: > TESTS=ext/standard/tests/file/tempnam_suffix.phpt make test
That should be fine. Chris
> > Is there a better way to do it? > > > Kind regards, > Stefan >
-- christopher.jones@oracle.com http://twitter.com/ghrd Free PHP & Oracle book: http://www.oracle.com/technetwork/topics/php/underground-php-oracle-manual-098250.html

Stefan Neufeind

12 years ago
On 01/30/2014 08:08 PM, Christopher Jones wrote:
> > > On 01/30/2014 10:55 AM, Stefan Neufeind wrote: >> Hi, >> >> On 01/30/2014 07:08 PM, Christopher Jones wrote: >>> >>> On 01/30/2014 01:11 AM, Stefan Neufeind wrote: >>>> On 01/12/2014 09:40 PM, Johannes Schlüter wrote: >>>>> On Sat, 2014-01-11 at 20:15 +0100, Stefan Neufeind wrote: >>>>>> -static int php_do_open_temporary_file(const char *path, const char >>>>>> *pfx, char **opened_path_p TSRMLS_DC) >>>>>> +static int php_do_open_temporary_file(const char *path, const char >>>>>> *pfx, char **opened_path_p TSRMLS_DC, const char *suffix) >>>>> >>>>> TSRM should be the last parameter, move your new one in front of it. >>>>> >>>>>> -PHPAPI int php_open_temporary_fd_ex(const char *dir, const char >>>>>> *pfx, char **opened_path_p, zend_bool open_basedir_check TSRMLS_DC) >>>>>> +PHPAPI int php_open_temporary_fd_ex(const char *dir, const char >>>>>> *pfx, char **opened_path_p, zend_bool open_basedir_check TSRMLS_DC, >>>>>> const char *suffix) >>>>> >>>>>> -PHPAPI int php_open_temporary_fd(const char *dir, const char *pfx, >>>>>> char **opened_path_p TSRMLS_DC) >>>>>> +PHPAPI int php_open_temporary_fd(const char *dir, const char *pfx, >>>>>> char **opened_path_p TSRMLS_DC, const char *suffix) >>>>> >>>>>> -PHPAPI FILE *php_open_temporary_file(const char *dir, const char >>>>>> *pfx, char **opened_path_p TSRMLS_DC) >>>>>> +PHPAPI FILE *php_open_temporary_file(const char *dir, const char >>>>>> *pfx, char **opened_path_p TSRMLS_DC, const char *suffix) >>>>> [...] >>>>> >>>>> These are API changes so it could only be in 5.6.0. Eventually it >>>>> might >>>>> be added if instead of changing APIs new functions might be introduced >>>>> and the old ones call the new ones. >>>>> (also TSRM should be last last there, too) >>>>> >>>>> Aside for that: A test would be good. >>>>> >>>>> For not loosing it either create a feature request at bugs.php.net or >>>>> pull request at github. >>>> >>>> Hi, >>>> >>>> I've sent a reply to Ferenc and David some days ago. But maybe somebody >>>> else could also help get this forward a bit. >>>> >>>> RFC is here, together with a pull-request: >>>> https://wiki.php.net/rfc/tempnam-suffix >>>> >>>> I also tried creating a testcase. For some reason it fails - but from >>>> the commandline I don't see why. I guess it's because my ".png"-example >>>> fails to validate against the expected result or something? >>>> The actual output of the script for testing is fine for me though. >>>> >>>> Would be awesome if somebody could pick this up please. >>> >>> The RFC needs some basic info, e.g. is there a default for the >>> optional suffix? What are any potential porting issues for users >>> (or for the implementation)? Also, make it easy for readers by >>> giving an >>> example of what happens now and what happens with the new >>> implementation. >> >> Well, it's an additional option. If you don't give it, it behaves as >> before. And as commonly with a suffix: If you don't specify one, you >> don't get a suffix. >> >> Sorry I don't know what is unclear. But maybe it's just an >> experience-thing how "explicitly" you usually handle such things in an >> RFC. > > Prior PHP RFCs are not the best examples of RFCs in the world. Feel free > to do better! > > One rule would be if something has to be explained on the mail list, then > it should be in the RFC. > > Also, from experience, RFCs should state what they do, and also what they > don't do. It's better not to leave any gray areas. > > Don't forget to have content that can be reused for the PHP manual > (See #8 in > https://blogs.oracle.com/opal/entry/the_mysterious_php_rfc_process).
I'll shortly try to write at least a small bit more. Thanks for your pointers.
>>> Having a failing testcase isn't going to help tempt someone to spend >>> time merging the code. Perhaps you can revisit this and get it >>> working? (Or state that if the RFC is accepted, you will do the work >>> to get it working). Are any existing tests broken by the change? >> >> I tried to find out, but didn't manage to get a "verbose" output of the >> testcase-runner on the console. It just said "failed" for me but didn't >> give me the returned / expected output or so. So it's more a question of >> "how to get it right". The testcode in the test itself works. I suspect >> a notation-thing that for somebody experienced will be peanuts. > > I'd never take that for granted. It will take some fiddling, I'm sure. > I can understand you might not want to spend time on it if there is a > possibility the RFC isn't going to be accepted. However I don't see > your RFC as something that would fail to pass a vote. > > Check the .log, .exp files etc from the test run and post questions > on the mail list. > >> >> I currently run it with: >> TESTS=ext/standard/tests/file/tempnam_suffix.phpt make test > > That should be fine.
Thanks to Gernot Vormayr it works now. "--show-all" was what I missed. Passing it as part of "TESTS" was what shows the small glitch. Test working as well now. Kind regards, Stefan

Stefan Neufeind

12 years ago
Hi, I'd like to pick up the original discussion about this patch here again. There have some updates on the github-pull. Maybe somebody could have a look into this, please? https://github.com/php/php-src/pull/575 Kind regards, Stefan

Ferenc Kovacs

12 years ago
On Sat, Apr 12, 2014 at 12:24 AM, Stefan Neufeind <neufeind@php.net> wrote:
> Hi, > > I'd like to pick up the original discussion about this patch here again. > There have some updates on the github-pull. Maybe somebody could have a > look into this, please? > > https://github.com/php/php-src/pull/575 > > > Kind regards, > Stefan > > -- > PHP Internals - PHP Runtime Development Mailing List > To unsubscribe, visit: http://www.php.net/unsub.php > >
Just to make it clear: To make it into 5.6.0 the RFC should have been voted and accepted, and the patch finished and merged before the release of the first beta. None of that happened in time, so now there is no chance to be in 5.6.0.
-- Ferenc Kovács @Tyr43l - http://tyrael.hu

Athos Ribeiro

3 years ago
On Sat, Apr 12, 2014 at 06:33:40AM +0000, Ferenc Kovacs wrote:
>On Sat, Apr 12, 2014 at 12:24 AM, Stefan Neufeind <neufeind@php.net> wrote: > >> Hi, >> >> I'd like to pick up the original discussion about this patch here again. >> There have some updates on the github-pull. Maybe somebody could have a >> look into this, please? >> >> https://github.com/php/php-src/pull/575 >> >> >> Kind regards, >> Stefan >>
>Just to make it clear: >To make it into 5.6.0 the RFC should have been voted and accepted, and the >patch finished and merged before the release of the first beta. >None of that happened in time, so now there is no chance to be in 5.6.0.
Hi, sorry for reviving this old thread. Is there still interest in pushing the related RFC [1] forward? This thead started few years ago at [2] and has not seen much activity since the proposed change [3] stalled due to issues with test failures. I also found a couple old bugs [4,5] which also requested adding the related feature but they also have not seen any activity since then. I have a first implementation of the feature at [6], but it does have a few points which would need discussion (most likely should go in the RFC). Namely, - the new suffix arg mimics the prefix arg when path separators are present in the string (goes through basename, using only the last part of the provided path); - the suffix gets truncated if its length is longer than 64 characters (also matching the prefix behavior); and - the current proposed patch does not include a windows implementation. I could not find the historical reasons behind the prefix behavior described in the first two points. Then, I was unsure if they should be kept for the suffix. I then took a conservative approach and kept them just to match the prefix arg behavior here. As for not including windows support, the reason lies in the underlying function (and API) used to generate temporary files, which do not provide means to set a suffix and also include a .TMP extension to the files, which AFAICT, does matter in windows systems. I also found a related discussion in an old bug at [7]. I am Ccing the original author here to make sure they get this message so I can understand if they are willing to move forward with the RFC or if I should file a new one (as per the first note in [8]). Finally, as per past discussions on this thread, I understand it is too late to include this change in 8.3, and given the api change, this should be deferred to 8.4 in case an RFC is accepted. [1] https://wiki.php.net/rfc/tempnam-suffix [2] https://marc.info/?l=php-internals&m=138946779304541 [3] https://github.com/php/php-src/pull/575 [4] https://bugs.php.net/bug.php?id=37613 [5] https://bugs.php.net/bug.php?id=43898 [6] https://github.com/php/php-src/pull/11685 [7] https://bugs.php.net/bug.php?id=44222 [8] https://wiki.php.net/rfc/howto
-- Athos Ribeiro