Re: Re: Please review: new output control implementation

Marcus Boerger wrote:

I'd say the only thing that doesn't look ten times better than what we have right now is that you have exported function prefixed with underscore "PHPAPI int _php_output" and static ones without. Imo it should be the other way round.

This is because the TSRM macros don't have underscores, so one uses php_output_flush() instead of _php_output_flush(TSRMLS_C) of course. So the public API except that used in main.c or in SAPIs is TSRM macro free (like in the streams layer or pecl/http and possibly else where).

The open question i have however is how do you deal with unicode. I saw some estr*() calls where i would have expected a few unicode checks. So there is no unicode support right now. So i suggest you add that too.

I don't yet see why output control should know about or handle unicode differently, it basically just shovels binary data around. I didn't dive into the unicode stuff yet, so I might be quite off...

A think i am a bit curious yout is why some function have both *() and *_all() versions. Why didn't you choose to pass an int in?

Because the *_all() functions don't honor if the handler is cleanable and/or removable, those are ultimate functions like for shutting down the output control layer. I think it's better to have separate functions to emphasize this difference.

Last but not least i wonder why i am seeing use of the strange TSRMLS_FETCH_FROM_CTX() instead of passing TSRM keys using TSRMLS_* macros.

This is only used in zend_stack_apply callbacks which don't support TSRM by signature. Thanks, -- Michael - <mike(@)php.net> http://dev.iworks.at/ext-http/http-functions.html.gz