Fw: setStub inconsistencies - dubious patch attached
| From: | Steph Fox | Date: | Thu, 31 Jan 2008 15:05:54 +0000 |
| Subject: | Fw: setStub inconsistencies - dubious patch attached | ||
| Groups: | php.pecl.dev | ||
| Request: | Send a blank email to pecl-dev+get-5095@lists.php.net to get a copy of this message | ||
Trying again on-list (hello Pierre).
- Steph
----- Original Message ----- From: "Steph Fox" <steph@zend.com>
To: "Greg Beaver" <greg@chiaraquartet.net>; "Marcus Boerger" <mail@marcus-boerger.de>
Sent: Thursday, January 31, 2008 4:30 AM
Subject: setStub inconsistencies - dubious patch attached
Hi both, I went to look into the tarmaker issues earlier, and now have shitloads of questions about the implementation of setStub(). That's what happens when you send me to look at failing tests, see... To start with there are exceptions all the way through that method, which seems like a pretty bad idea given that it's supposed to return a Boolean value. Shouldn't there be a bog-standard E_WARNING on failure? And why would you want to throw an exception - which for most mortals causes a fatal error - just because you couldn't set the stub to anything other than default? Also, when you rely on exceptions, calling setStub() with the wrong data can result in a completely empty phar because there's no way to return FALSE. So I don't think exceptions work well there, given that this code is supposed to be easy for other people to use anyways. I don't see the point of passing in a length to setStub(), optionally or otherwise - is there some reason for not just passing -1 to phar_flush()? or is it a BC thing? (I can't remember if/how setStub() was before.) It'd be easy enough to put it back, I just took it out because it looked so pointless. Another problem is the quiet parameter parsing for resources, which means that any parameter-parsing errors will mention the single-parameter string option and nothing else. Confusing, if you happen to be working with file pointers... Finally, there's an error 'unable to read from input stream' which I couldn't reach to test. I'm really not sure what you have to do to get that warning, so I left it in. So - the attached version's a fair bit longer than the original (not least because I like lots of ws) but it does at least return TRUE when the method's had the intended effect, FALSE otherwise, doesn't wreck the phar in the process, and tells you precisely what went wrong if anything goes awry. It's probably not very fashionable tho'. Let me know what you think. - Steph
/* {{{ proto bool Phar::setStub([string|stream stub]) * Change the stub of the archive */ PHP_METHOD(Phar, setStub) { zval *zstub = NULL; zval ***varargs = NULL; char *error, *stub = NULL; int num_varargs = 0; int stub_len = 0; php_stream *stream; PHAR_ARCHIVE_OBJECT(); if (PHAR_G(readonly)) { php_error_docref(NULL TSRMLS_CC, E_WARNING, "Cannot set stub, phar is read-only"); RETURN_FALSE; } if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "*", &varargs, &num_varargs) == SUCCESS) { if (ZEND_NUM_ARGS() > 1) { php_error_docref(NULL TSRMLS_CC, E_WARNING, "%s expects no more than 1 parameter, %d given", get_active_function_name(TSRMLS_C), num_varargs); RETURN_FALSE; } if (num_varargs == 0 || Z_TYPE_PP(varargs[0]) == IS_STRING || Z_TYPE_PP(varargs[0]) == IS_NULL) { stub = varargs ? Z_STRVAL_PP(varargs[0]) : NULL; if (stub) { stub_len = strlen(stub); phar_flush(phar_obj->arc.archive, stub, stub_len, &error TSRMLS_CC); if (error) { php_error_docref(NULL TSRMLS_CC, E_WARNING, "%s", error); efree(error); RETURN_FALSE; } RETURN_TRUE; } } else if (Z_TYPE_PP(varargs[0]) == IS_RESOURCE) { zstub = *varargs[0]; if ((php_stream_from_zval_no_verify(stream, &zstub)) != NULL) { phar_flush(phar_obj->arc.archive, (char *) &zstub, -1, &error TSRMLS_CC); if (error) { php_error_docref(NULL TSRMLS_CC, E_WARNING, "%s", error); efree(error); RETURN_FALSE; } RETURN_TRUE; } else { // is this really necessary? I couldn't find a way to trigger it.. php_error_docref(NULL TSRMLS_CC, E_WARNING, "Cannot set stub, unable to read from input stream"); } } else { php_error_docref(NULL TSRMLS_CC, E_WARNING, "%s expects either a string or a resource, %s given", get_active_function_name(TSRMLS_C), zend_get_type_by_const(Z_TYPE_PP(varargs[0]))); } } if (varargs) { efree(varargs); } RETURN_FALSE; } /* }}} */