Re: Suggestion: Add optional suffix to tempnam()

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