Hey,
it has come to my attention that stream_context_get_default() is /
grossly/ misnamed, considering
that it can be used (and is the only way) to SET default options. It
works more like ini_set() in that
you pass in the new defaults and it returns the old.
With the current name, it is as bad as if we added an optional
argument to ini_get() that would also
set the value.
I know we're already into PHP 5.3 alpha, but this is a simple alias,
so I'm hoping you can squeeze it in.
If not, I understand, and will leave that decision to the RM :)
- Davey
Hey,
it has come to my attention that
stream_context_get_default()is /grossly/
misnamed, considering
that it can be used (and is the only way) to SET default options. It works
more likeini_set()in that
you pass in the new defaults and it returns the old.With the current name, it is as bad as if we added an optional argument to
ini_get()that would also
set the value.I know we're already into PHP 5.3 alpha, but this is a simple alias, so I'm
hoping you can squeeze it in.
That doesn't really solve anything as the argument is optional. In
fact, to me it reads like the default context would be reset to
nothing.
$ctx = stream_context_set_default();
Doesn't look any less weird than stream_context_get_default($ctx);
I do agree though. The naming convention of the stream_*() functions
are extremely bad.
-Hannes
Hey,
it has come to my attention that
stream_context_get_default()
is /grossly/ misnamed, considering
[...]
That doesn't really solve anything as the argument is optional.
Adding a new function which enforces the parameter sounds way better
then a simple alias.
In
fact, to me it reads like the default context would be reset to
nothing.
I hope not ... I'd expect it to return the current default context
unchanged, else that function certainly should be replaced....
johannes
Hey,
it has come to my attention that
stream_context_get_default()
is /grossly/ misnamed, considering
[...]
That doesn't really solve anything as the argument is optional.Adding a new function which enforces the parameter sounds way better
then a simple alias.
I agree.
In
fact, to me it reads like the default context would be reset to
nothing.I hope not ... I'd expect it to return the current default context
unchanged, else that function certainly should be replaced....
Read the example again: $ctx = stream_context_set_default(); (using
the "new alias") :)
using stream_context_get_default() without passing context to it
returns the default context.
-Hannes
I'll see if I can write this as a patch instead; I was trying for
"path of least resistance"
Should be a fun exercise for me! Will get to it tonight.
- Davey
On Mon, Aug 4, 2008 at 7:14 AM, Hannes Magnusson
hannes.magnusson@gmail.com wrote:
Hey,
it has come to my attention that
stream_context_get_default()
is /grossly/ misnamed, considering
[...]
That doesn't really solve anything as the argument is optional.Adding a new function which enforces the parameter sounds way better
then a simple alias.I agree.
In
fact, to me it reads like the default context would be reset to
nothing.I hope not ... I'd expect it to return the current default context
unchanged, else that function certainly should be replaced....Read the example again: $ctx = stream_context_set_default(); (using
the "new alias") :)using stream_context_get_default() without passing context to it
returns the default context.-Hannes
OK, here's an attempt at a patch[1], I discussed it briefly with
Johannes
and he felt some discussion was needed with regards to the return value.
I personally seem some benefit to returning the "new" context; Johannes
wasn't sure that returning "true" might not be a better option in that
we
cannot return the previous "value" like ini_set().
I had suggested a second optional argument that could be assigned the
resource (context), and then return true. Though I still think that
returning
the resource is the best option.
- Davey
[1] http://pixelated-dreams.com/~davey/stream_context_set_default.patch
On Mon, Aug 4, 2008 at 13:02, Johannes Schlüter johannes@php.net
wrote:Hey,
it has come to my attention that
stream_context_get_default()
is /grossly/ misnamed, considering
[...]
That doesn't really solve anything as the argument is optional.Adding a new function which enforces the parameter sounds way better
then a simple alias.I agree.
In
fact, to me it reads like the default context would be reset to
nothing.I hope not ... I'd expect it to return the current default context
unchanged, else that function certainly should be replaced....Read the example again: $ctx = stream_context_set_default(); (using
the "new alias") :)using stream_context_get_default() without passing context to it
returns the default context.-Hannes
I had suggested a second optional argument that could be assigned the
resource (context), and then return true. Though I still think that
returning
the resource is the best option.
Runs and compiles fine from here. I like the idea too. Great first attempt :)
Slan,
David
OK, here's an attempt at a patch[1], I discussed it briefly with Johannes
and he felt some discussion was needed with regards to the return value.I personally seem some benefit to returning the "new" context; Johannes
wasn't sure that returning "true" might not be a better option in that we
cannot return the previous "value" likeini_set().
Why can't we?
If there was a default context then return it, otherwise true/false..
Hmh. That could be confusing.
I'm fine returning the new context :)
[1] http://pixelated-dreams.com/~davey/stream_context_set_default.patch
The arginfo is wrong, the parameter is required, not optional.
- parse_context_options(context, params TSRMLS_CC);
params? Shouldn't this be options? :)
-Hannes
OK, here's an attempt at a patch[1], I discussed it briefly with
Johannes
and he felt some discussion was needed with regards to the return
value.I personally seem some benefit to returning the "new" context;
Johannes
wasn't sure that returning "true" might not be a better option in
that we
cannot return the previous "value" likeini_set().Why can't we?
If there was a default context then return it, otherwise true/false..
Hmh. That could be confusing.
I'm fine returning the new context :)
There is always a default context :)
[1] http://pixelated-dreams.com/~davey/stream_context_set_default.patch
The arginfo is wrong, the parameter is required, not optional.
I don't see this, but that's just me not knowing, I thought that
anything following
a | (pipe) was optional, otherwise it was implicitly required,
how do you make an argument explicitly required if that is not the case?
- parse_context_options(context, params TSRMLS_CC);
The patch has been update to fix the params/options mistake,
Still at http://pixelated-dreams.com/~davey/stream_context_set_default.patch
- Davey
OK, here's an attempt at a patch[1], I discussed it briefly with Johannes
and he felt some discussion was needed with regards to the return value.I personally seem some benefit to returning the "new" context; Johannes
wasn't sure that returning "true" might not be a better option in that we
cannot return the previous "value" likeini_set().Why can't we?
If there was a default context then return it, otherwise true/false..
Hmh. That could be confusing.
I'm fine returning the new context :)There is always a default context :)
[1] http://pixelated-dreams.com/~davey/stream_context_set_default.patch
The arginfo is wrong, the parameter is required, not optional.
I don't see this, but that's just me not knowing, I thought that anything
following
a | (pipe) was optional, otherwise it was implicitly required,
how do you make an argument explicitly required if that is not the case?
Correct, but ext/reflaction doesn't read the zend_parse_parameters(),
it reads the arginfos, so those have to match the
zend_parse_parameters() statement.
ZEND_BEGIN_ARG_INFO_EX(arginfo_stream_context_set_default, 0, 0, 0)
should be
ZEND_BEING_ARG_INF(arginfo_stream_context_set_default, 0)
(meaning all ZEND_ARG_INFO() arguments are required)
or
ZEND_BEGIN_ARG_INFO_EX(arginfo_stream_context_set_default, 0, 0, 1)
(meaning only the first ZEND_ARG_INFO() argument is required)
-Hannes
OK, here's an attempt at a patch[1], I discussed it briefly with
Johannes
and he felt some discussion was needed with regards to the return
value.I personally seem some benefit to returning the "new" context;
Johannes
wasn't sure that returning "true" might not be a better option in
that we
cannot return the previous "value" likeini_set().Why can't we?
If there was a default context then return it, otherwise true/
false..
Hmh. That could be confusing.
I'm fine returning the new context :)There is always a default context :)
[1] http://pixelated-dreams.com/~davey/stream_context_set_default.patch
The arginfo is wrong, the parameter is required, not optional.
I don't see this, but that's just me not knowing, I thought that
anything
following
a | (pipe) was optional, otherwise it was implicitly required,
how do you make an argument explicitly required if that is not the
case?Correct, but ext/reflaction doesn't read the zend_parse_parameters(),
it reads the arginfos, so those have to match the
zend_parse_parameters() statement.ZEND_BEGIN_ARG_INFO_EX(arginfo_stream_context_set_default, 0, 0, 0)
should be
ZEND_BEING_ARG_INF(arginfo_stream_context_set_default, 0)
(meaning all ZEND_ARG_INFO() arguments are required)
or
ZEND_BEGIN_ARG_INFO_EX(arginfo_stream_context_set_default, 0, 0, 1)
(meaning only the first ZEND_ARG_INFO() argument is required)-Hannes
Done, thank you for the explanation :)
- Davey
OK, here's an attempt at a patch[1], I discussed it briefly with
Johannes
and he felt some discussion was needed with regards to the return
value.I personally seem some benefit to returning the "new" context;
Johannes
wasn't sure that returning "true" might not be a better option in
that we
cannot return the previous "value" likeini_set().Why can't we?
If there was a default context then return it, otherwise true/
false..
Hmh. That could be confusing.
I'm fine returning the new context :)There is always a default context :)
[1] http://pixelated-dreams.com/~davey/stream_context_set_default.patch
The arginfo is wrong, the parameter is required, not optional.
I don't see this, but that's just me not knowing, I thought that
anything
following
a | (pipe) was optional, otherwise it was implicitly required,
how do you make an argument explicitly required if that is not the
case?Correct, but ext/reflaction doesn't read the zend_parse_parameters(),
it reads the arginfos, so those have to match the
zend_parse_parameters() statement.ZEND_BEGIN_ARG_INFO_EX(arginfo_stream_context_set_default, 0, 0, 0)
should be
ZEND_BEING_ARG_INF(arginfo_stream_context_set_default, 0)
(meaning all ZEND_ARG_INFO() arguments are required)
or
ZEND_BEGIN_ARG_INFO_EX(arginfo_stream_context_set_default, 0, 0, 1)
(meaning only the first ZEND_ARG_INFO() argument is required)-Hannes
oops, patch is still at: http://pixelated-dreams.com/~davey/stream_context_set_default.patch
- Davey
oops, patch is still at:
http://pixelated-dreams.com/~davey/stream_context_set_default.patch
Committed.
Care to write the docs for it too? :)
-Hannes