Re: Suggestion: Add optional suffix to tempnam()

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