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