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/
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/
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.
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/
On Sun, 11 Sep 2011 20:26:20 +0100, Sebastian Bergmann sebastian@php.net
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
Hi,
On Sun, 11 Sep 2011 20:26:20 +0100, Sebastian Bergmann sebastian@php.net
wrote:[...]
[1] https://wiki.php.net/rfc/streamwrapper-factory
[2] http://schlueters.de/~johannes/php/stream_factory.diffA 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
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/
Em Mon, 12 Sep 2011 12:53:13 +0100, Sebastian Bergmann sebastian@php.net
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
Em Mon, 12 Sep 2011 12:53:13 +0100, Sebastian Bergmann
sebastian@php.net 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.
Not sure if it's actual, but this behaviour is documented:
http://de3.php.net/manual/en/streamwrapper.construct.php#94731
Christian
Em Mon, 12 Sep 2011 15:40:20 +0100, Christian Kaps
christian.kaps@mohiva.com escreveu:
Em Mon, 12 Sep 2011 12:53:13 +0100, Sebastian Bergmann
sebastian@php.net 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.
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
Em Mon, 12 Sep 2011 15:40:20 +0100, Christian Kaps
christian.kaps@mohiva.com escreveu:Em Mon, 12 Sep 2011 12:53:13 +0100, Sebastian Bergmann
sebastian@php.net 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.
Not sure if it's actual, but this behaviour is documented:
http://de3.php.net/manual/en/streamwrapper.construct.php#94731Ah, right.
stat()andunlink()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
On Mon, 12 Sep 2011 18:00:13 +0100, Hannes Magnusson
hannes.magnusson@gmail.com wrote:
On Mon, Sep 12, 2011 at 16:56, Gustavo Lopes glopes@nebm.ist.utl.pt
wrote:Ah, right.
stat()andunlink()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.
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 thestream_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
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
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!
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--
--
Pierre
@pierrejoye | http://blog.thepimp.net | http://www.libgd.org