# [RFC] Factory for Stream Wrappers - Thread: https://externals.io/message/55365 - Original archive: https://news-web.php.net/php.internals/55365 - Started by Sebastian Bergmann on 2011-09-11T19:26:20+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 --- ## #55365 — Sebastian Bergmann — 2011-09-11T19:26:20+00:00 URL: https://externals.io/message/55365#55365 We (Arne Blankerts, Stefan Priebsch, Benjamin Eberlei, and I) have worked on/with code where a factory for stream wrappers would be very helpful. Earlier this year we talked to Ilia about this at ConFoo and he was open to the idea but did not have the time to implement it. At FrOSCon we talked to Johannes about this and he volunteered to implement it; which he did. We now have an RFC at [1] and an initial patch at [2]. Benjamin will write tests in the next couple of days. We know that we are "late to the party" but we do think that this would be a good addition to PHP 5.4. -- [1] https://wiki.php.net/rfc/streamwrapper-factory [2] http://schlueters.de/~johannes/php/stream_factory.diff -- Sebastian Bergmann Co-Founder and Principal Consultant http://sebastian-bergmann.de/ http://thePHP.cc/ --- ## #55370 — Anthony Ferrara — 2011-09-11T21:40:56+00:00 URL: https://externals.io/message/55365#55370 · In reply to: #55365 So, based on the rfc, does "must return an instance" mean that it can't throw an exception (ex: if a resource couldn't be opened)? > We (Arne Blankerts, Stefan Priebsch, Benjamin Eberlei, and I) have > worked on/with code where a factory for stream wrappers would be very > helpful. > > Earlier this year we talked to Ilia about this at ConFoo and he was open > to the idea but did not have the time to implement it. At FrOSCon we > talked to Johannes about this and he volunteered to implement it; which > he did. We now have an RFC at [1] and an initial patch at [2]. Benjamin > will write tests in the next couple of days. > > We know that we are "late to the party" but we do think that this would > be a good addition to PHP 5.4. > > -- > [1] https://wiki.php.net/rfc/streamwrapper-factory > [2] http://schlueters.de/~johannes/php/stream_factory.diff > > -- > Sebastian Bergmann Co-Founder and Principal Consultant > http://sebastian-bergmann.de/ http://thePHP.cc/ --- ## #55371 — Sebastian Krebs — 2011-09-11T21:47:23+00:00 URL: https://externals.io/message/55365#55371 · In reply to: #55370 Hi, Am 11.09.2011 23:40, schrieb Anthony Ferrara: > So, based on the rfc, does "must return an instance" mean that it can't > throw an exception (ex: if a resource couldn't be opened)? A resource is opened on `streamWrapper::dir_opendir()` or `streamWrapper::stream_open()` anyway, but not on Wrapper-instanciation. > > > […] --- ## #55373 — Gustavo Lopes — 2011-09-11T22:26:07+00:00 URL: https://externals.io/message/55365#55373 · In reply to: #55365 On Sun, 11 Sep 2011 20:26:20 +0100, Sebastian Bergmann wrote: > [...] > -- > [1] https://wiki.php.net/rfc/streamwrapper-factory > [2] http://schlueters.de/~johannes/php/stream_factory.diff > A patch against trunk (or 5.4) would have been nicer. Other than that: * This patch has a huge BC: PHP_FUNCTION(stream_wrapper_register) { ... - if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "ss|l", &protocol, &protocol_len, &classname, &classname_len, &flags) == FAILURE) { + + uwrap = (struct php_user_stream_wrapper *)ecalloc(1, sizeof(*uwrap)); + + if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "sf|l", &protocol, &protocol_len, &uwrap->fci, &uwrap->fcc, &flags) == FAILURE) { + efree(uwrap); It should be changed so that the current form of stream_wrapper_register is still accepted. I assume this is a mistake because user_wrapper_get_object still tries to instantiate an object directly if there's no callback. * Calling the object handler get_class_name is nicer than using Z_OBJCE_P(us->object)->name) because it works with more objects. * You claim this has two advantages -- 1. Allows to inject state into stream wrappers from other parts of the application without having to access global state or encode information into the stream uri. 2. Increases testability of code using stream wrappers. Could you add some examples substantiating these claims? I have some doubts. If your factory is a function, #1 doesn't seem to apply. The only way I see you can retrieve state to inject is by exactly the same means available on the wrapper object constructor or stream_open (with the aggravating factor that in stream_open you have access to the context and to the URI, but in the factory you have access to none). If, however, the factory is an object method, I see that you could change state in the object and the factory method would have access to this new state (and could inject in the wrapper object). Claim #2 is more dubious. Adding a level of indirection in object creation is indeed useful for testing because you can inject to objects that need to instantiate other objects a different factory that will return mock objects. However, I don't see how this applies here. The stream wrapper already doesn't have a fixed dependency; it functions like a basic factory, instantiating an object of a class you give. You can already swap the implementation of a stream wrapper by varying the class name upon registration. So I don't see how you gain anything with this extra complexity. -- Gustavo Lopes --- ## #55374 — johannes@schlueters.de — 2011-09-11T22:51:08+00:00 URL: https://externals.io/message/55365#55374 · In reply to: #55373 Hi, > On Sun, 11 Sep 2011 20:26:20 +0100, Sebastian Bergmann > wrote: > > > […] > > A patch against trunk (or 5.4) would have been nicer. Other than that: > > * This patch has a huge BC: Yes, that's not the latest version of the patch but the proof of concept one. The latest version will be uploaded tomorrow evening my time. johannes --- ## #55382 — Sebastian Bergmann — 2011-09-12T11:53:13+00:00 URL: https://externals.io/message/55365#55382 · In reply to: #55373 Am 12.09.2011 00:26, schrieb Gustavo Lopes: > A patch against trunk (or 5.4) would have been nicer. Other than that: > * This patch has a huge BC: Johannes already said that he is updating his patch. It is not our intention to break BC, we only want to add additional/optional behaviour. > Could you add some examples substantiating these claims? I have some doubts. No matter how you implement the actual instantiation code in user land it should be obvious that you always have more control over it than having PHP do it for you in a "static" way. The usual case for a factory of course would be having an object instance where a method would be called - passing in array($obj, 'method') as the callback. But even a classic function has more control already, since it can decide which class to actually instantiate and what parameters it might need to set. Regarding state it is important to notice that PHP does *not* execute the constructor on all low level calls when instantiating the wrapper class - for whatever reason that is the case. Changing that behaviour would cause quite some side effects, with possible quite some BC breaks. > If, however, the factory is an object method, I see that you could change > state in the object and the factory method would have access to this new > state (and could inject in the wrapper object). Do you still need a use case for this? The main use case for Stefan and Arne was that we want to use a wrapper to map paths into protocols (e.g. have foo:// me mapped to $basedir . '/foo/') or define a memcached:// streamwrapper that obviously needs a server to talk to. This is usually a configuration that would be injected into the factory and from there be passed on to the instance. I do not have an example at hand that shows how this feature will help with testability, for that I am sorry. Maybe Benjamin can elaborate on what his use case is. -- Sebastian Bergmann Co-Founder and Principal Consultant http://sebastian-bergmann.de/ http://thePHP.cc/ --- ## #55385 — Gustavo Lopes — 2011-09-12T13:43:33+00:00 URL: https://externals.io/message/55365#55385 · In reply to: #55382 Em Mon, 12 Sep 2011 12:53:13 +0100, Sebastian Bergmann escreveu: > Regarding state it is important to notice that PHP does *not* execute > the constructor on all low level calls when instantiating the wrapper > class - for whatever reason that is the case. Changing that behaviour > would cause quite some side effects, with possible quite some BC breaks. This is a bit off-topic, but could you elaborate on this (possibly submitting a bug report)? I see user_wrapper_opener trying to call the constructor: http://lxr.php.net/opengrok/xref/PHP_5_3/main/streams/userspace.c#298 So any failure to do this would be a bug. BTW, another advantage of the proposed change is that you can have stream wrappers with constructors that take arguments. For the record, I'm in favor of this, even though I don't buy the testability claim. My point yesterday was mainly the RFC did not explain the advantages very clearly. -- Gustavo Lopes --- ## #55386 — Christian Kaps — 2011-09-12T14:40:20+00:00 URL: https://externals.io/message/55365#55386 · In reply to: #55385 > Em Mon, 12 Sep 2011 12:53:13 +0100, Sebastian Bergmann > escreveu: > > > […] > > This is a bit off-topic, but could you elaborate on this (possibly > submitting a bug report)? I see user_wrapper_opener trying to call > the > constructor: > > http://lxr.php.net/opengrok/xref/PHP_5_3/main/streams/userspace.c#298 > > So any failure to do this would be a bug. > Not sure if it's actual, but this behaviour is documented: http://de3.php.net/manual/en/streamwrapper.construct.php#94731 Christian --- ## #55387 — Gustavo Lopes — 2011-09-12T14:56:40+00:00 URL: https://externals.io/message/55365#55387 · In reply to: #55386 Em Mon, 12 Sep 2011 15:40:20 +0100, Christian Kaps escreveu: > > > […] > > Not sure if it's actual, but this behaviour is documented: > http://de3.php.net/manual/en/streamwrapper.construct.php#94731 > Ah, right. `stat()` and `unlink()` should have been static in the first place. Given the circumstances, it might be a good idea, to document url_stat, unlink, rename, mkdir and rmdir to be static and change the implementation to: * not instantiate an object if the method is static * keep the current behavior if it's not static (but emit E_STRICT) -- Gustavo Lopes --- ## #55388 — Hannes Magnusson — 2011-09-12T17:00:13+00:00 URL: https://externals.io/message/55365#55388 · In reply to: #55387 > Em Mon, 12 Sep 2011 15:40:20 +0100, Christian Kaps > escreveu: > > > […] > > Ah, right. `stat()` and `unlink()` should have been static in the first place. > > Given the circumstances, it might be a good idea, to document url_stat, > unlink, rename, mkdir and rmdir to be static and change the implementation > to: > > * not instantiate an object if the method is static > * keep the current behavior if it's not static (but emit E_STRICT) That seems like a change in behavior just to change it. Continuing on the off-topic notes.. I would rather go with an alternative implementation based upon interfaces (i.e. dirwrapper, filesystemwrapper, streamwrapper...) and spec it out before adding random magical things that took over 7 years for people to figure out and document. And lets not mention the `stream_filter_register()` and `stream_bucket_new()` voodoo. -Hannes --- ## #55392 — Gustavo Lopes — 2011-09-12T22:45:04+00:00 URL: https://externals.io/message/55365#55392 · In reply to: #55388 On Mon, 12 Sep 2011 18:00:13 +0100, Hannes Magnusson wrote: > On Mon, Sep 12, 2011 at 16:56, Gustavo Lopes > wrote: > > […] > > That seems like a change in behavior just to change it. It's a change in behavior so it makes sense. Those operations are wrapper operations and by their nature they are static operations, meaning a stream instance is not required. See the difference between php_stream_ops and php_stream_wrapper_ops. It would be a minor change. > Continuing on the off-topic notes.. > I would rather go with an alternative implementation based upon > interfaces (i.e. dirwrapper, filesystemwrapper, streamwrapper...) and > spec it out before adding random magical things that took over 7 years > for people to figure out and document. > And lets not mention the `stream_filter_register()` and > `stream_bucket_new()` voodoo. > The stream filtering API is a failed feature, IMO. It has the complexity of APR buckets and brigades without their performance benefits (memory is still copied all the time) -- and of course it's not properly documented. -- Gustavo Lopes --- ## #55393 — Stas Malyshev — 2011-09-12T22:55:22+00:00 URL: https://externals.io/message/55365#55393 · In reply to: #55392 Hi! > It's a change in behavior so it makes sense. Those operations are wrapper > operations and by their nature they are static operations, meaning a > stream instance is not required. See the difference between php_stream_ops > and php_stream_wrapper_ops. It would be a minor change. I must say I am feeling really uneasy about "minor changes" days before beta. For later though it may make sense... -- Stanislav Malyshev, Software Architect SugarCRM: http://www.sugarcrm.com/ (408)454-6900 ext. 227 --- ## #55394 — Pierre Joye — 2011-09-12T23:17:34+00:00 URL: https://externals.io/message/55365#55394 · In reply to: #55393 hi! I like the idea to improve this area. However I'm with Stas here. I do not feel comfortable enough with this RFC to agree to get into 5.4 at this stage. It is a sensible area and we are already late (because of the tests) with the 5.4 release plan. Next will begin in spring next year, not too far away from now :) Cheers, > Hi! > > > > […] > > I must say I am feeling really uneasy about "minor changes" days before > beta. For later though it may make sense... > -- > Stanislav Malyshev, Software Architect > SugarCRM: http://www.sugarcrm.com/ > (408)454-6900 ext. 227 > > -- > > > > -- Pierre @pierrejoye | http://blog.thepimp.net | http://www.libgd.org