Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:55373
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: error (pb1.pair.com: domain nebm.ist.utl.pt from 193.136.128.22 cause and error)
Content-Type: text/plain; charset=utf-8; format=flowed; delsp=yes
To: internals@lists.php.net
References: <4E6D0B5C.8030907@php.net>
Date: Sun, 11 Sep 2011 23:26:07 +0100
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Organization: =?utf-8?Q?N=C3=BAcleo_de_Eng=2E_Biom=C3=A9di?=
 =?utf-8?Q?ca_do_IST?=
Message-ID: <op.v1n0lttyidpuyk@cataphract.cata.lo.geleia.net>
In-Reply-To: <4E6D0B5C.8030907@php.net>
User-Agent: Opera Mail/11.51 (Win32)
Subject: Re: [PHP-DEV] [RFC] Factory for Stream Wrappers
From: glopes@nebm.ist.utl.pt ("Gustavo Lopes")

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