# [PATCH] Alias stream_context_get_default() as stream_context_set_default() - Thread: https://externals.io/message/39586 - Original archive: https://news-web.php.net/php.internals/39586 - Started by Davey Shafik on 2008-08-03T23:29:15+00:00 - Messages: 13 - Reply by email to internals@lists.php.net - More for agents: search with https://externals.io/search.md?q={query} · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt --- ## #39586 — Davey Shafik — 2008-08-03T23:29:15+00:00 URL: https://externals.io/message/39586#39586 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 --- ## #39599 — Hannes Magnusson — 2008-08-04T07:49:43+00:00 URL: https://externals.io/message/39586#39599 · In reply to: #39586 > 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. 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 --- ## #39624 — johannes@php.net — 2008-08-04T11:02:29+00:00 URL: https://externals.io/message/39586#39624 · In reply to: #39599 > > > […] [...] > 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 --- ## #39625 — Hannes Magnusson — 2008-08-04T11:14:35+00:00 URL: https://externals.io/message/39586#39625 · In reply to: #39624 > > > […] > [...] > > […] > > Adding a new function which enforces the parameter sounds way better > then a simple alias. I agree. > > > […] > > 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 --- ## #39636 — Davey Shafik — 2008-08-04T17:15:07+00:00 URL: https://externals.io/message/39586#39636 · In reply to: #39625 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 wrote: > > > […] > > I agree. > > > […] > > 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 --- ## #39683 — Davey Shafik — 2008-08-06T04:24:02+00:00 URL: https://externals.io/message/39586#39683 · In reply to: #39625 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 > wrote: > > […] > > I agree. > > > […] > > 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 --- ## #39684 — David Coallier — 2008-08-06T05:17:14+00:00 URL: https://externals.io/message/39586#39684 · In reply to: #39683 > 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 --- ## #39691 — Hannes Magnusson — 2008-08-06T07:47:13+00:00 URL: https://externals.io/message/39586#39691 · In reply to: #39683 > 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()`. 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 --- ## #39697 — Davey Shafik — 2008-08-06T13:11:52+00:00 URL: https://externals.io/message/39586#39697 · In reply to: #39691 > > > […] > > 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 :) > > > […] > > 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 --- ## #39701 — Hannes Magnusson — 2008-08-06T15:38:00+00:00 URL: https://externals.io/message/39586#39701 · In reply to: #39697 > > > > […] > > There is always a default context :) > > > […] > > 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 --- ## #39723 — Davey Shafik — 2008-08-07T01:42:33+00:00 URL: https://externals.io/message/39586#39723 · In reply to: #39701 > > > […] > > 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 --- ## #39724 — Davey Shafik — 2008-08-07T01:42:53+00:00 URL: https://externals.io/message/39586#39724 · In reply to: #39701 > > > […] > > 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 --- ## #39984 — Hannes Magnusson — 2008-08-16T11:05:55+00:00 URL: https://externals.io/message/39586#39984 · In reply to: #39724 > > 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