Re: [PHP-DEV] [VOTE] Object oriented session handlers
On 07/03/2011 03:24 AM, Arpad Ray wrote: Hi, Voting is now open for object oriented session handlers. The RFC and patch can be found here: https://wiki.php.net/rfc/session-oo You can vote here: https://wiki.php.net/rfc/session-oo/vote Regards, Arpad Hi, Although I have no voting karma, +1 BR, Steven -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
[PHP-DEV] Re: Big patch for FPM (config and initialization)
Sorry I always forget this mailing list is special and strip my attachments... Here is a link: http://pastebin.com/7JpXr22c On Sun, Jul 3, 2011 at 02:50, Giovanni Giacobbi giova...@giacobbi.net wrote: Greetings dear devs! A few days ago I made my first nginx+php-fpm setup, and I soon realized some gaps of the current FPM implementation. First of all, the lack of documentation, which of course I know I cannot complain about, but this forced me to dig into the source code which in turn motivated me to write this patch, which is good. The patch (attached) is for the current PHP 5.3 svn branch, but if you are interested in merging it in I will of course port it to 5.4 branch. I don't see particular reasons to apply it to 5.3, I was working on it because I'm planning to use it in my production environments. It would be really nice if it could make it in the 5.4 because it contains also some name changes in the ini file variables (with BC of course). The biggest and most interesting change is for sure the introduction of a [defaults] config section. More about this below, first the complete changelog of my patch: Detailed changelog of the patch: - Renamed options pm.status_path, ping.path, ping.response into a new logical group diagnostics, so they are now respectively: diagnostics.status_path, diagnostics.ping_path, diagnostics.ping_response. - Reordered in a more logical way the pool ini options, from a most-likely-to-be-customized first to the least. Usually when you edit config file you have big concentration on the first few settings, then you go like blah blah defaults defaults and so on, so this kind of order makes sense. - Aligned all the code listings of pool options with the official order to ease auditing. The official order is the one reported in the struct definition in fpm_conf.h, which of course is the same as the php-fpm.conf.in config template - Improved error messages. A better message helps new adopters to get started quickly, at the beginning I was really puzzled in front of some not very helpful messages. - Introduced a new section [defaults] that allows setting default values - Dropped restriction of not setting the same value multiple times, the last one holds - Removed a lot of redundant checks that are logically implied or not really required, without reducing the robustness of the program. - Improved a lot code readability in some parts, plus added some useful comments in the parts that were less easy to understand. - Refactored some functions to be shorter from code lines point of view, while still doing exactly the same function. - Various white space and cosmetic code improvements - Moved macros STR2STR, BOOL2STR, and PM2STR from fpm_conf.h to fpm_conf.c, no reason to make them public since that's the only file using them, so in case we need to change them in the future, there is less risk of breaking something which depends on them. - Fixed a memory leak in fpm_conf_expand_pool_name() (previous dynamically allocated *value was lost) Now about the new [defaults] section of the config file. In the current version if you want to make a decent looking config file you have to do something like that: [global] ...your global stuff... [pool1] user = pool1 listen = ... include = pool_defaults.conf [pool2] user = pool1 listen = ... include = pool_defaults.conf and so on, plus of course you need the external file pool_defaults.conf. With these changes, you can now do something like this: [global] ...your global stuff... [defaults] ...my defaults valid for every pool... pm.max_children = 500 pm.start_servers = 10 [pool1] user = pool1 listen = ... [pool2] user = pool2 listen = ... [pool3] user = pool3 listen = ... pm.start_servers = 20 There is also a small gain in the parsing time, because defaults are propagated in memory instead of parsed multiple times as with the include files solution. I have to admit this argument is quite irrilevant because it's only a startup time overhead, but it's so much more elegant in my eyes. Also by dropping the constraint of setting strings only once, you can override your defaults in the pools, so you can have an access log format for all of them except one. Upcoming changes that would follow from me if you accept this patch: 1) allow '$pool' variable to be used in the [globals], but this requires a bit of restructuring because it needs to be lazy-expanded in post process time instead of parsing time. 2) possibility to include more than one file per section Looking forward to have some feedback from you :) Kind regards -- Giovanni Giacobbi -- Giovanni Giacobbi -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Re: Big patch for FPM (config and initialization)
hi, It is nothing special, you have to use .txt for the attachment. It is also slightly better to create a feature requrest/report at bugs.php.net and attach the patch to it, so it won't get lost into the MLs archives :) Thanks for your work! Cheers, On Sun, Jul 3, 2011 at 3:16 PM, Giovanni Giacobbi giova...@giacobbi.net wrote: Sorry I always forget this mailing list is special and strip my attachments... Here is a link: http://pastebin.com/7JpXr22c On Sun, Jul 3, 2011 at 02:50, Giovanni Giacobbi giova...@giacobbi.net wrote: Greetings dear devs! A few days ago I made my first nginx+php-fpm setup, and I soon realized some gaps of the current FPM implementation. First of all, the lack of documentation, which of course I know I cannot complain about, but this forced me to dig into the source code which in turn motivated me to write this patch, which is good. The patch (attached) is for the current PHP 5.3 svn branch, but if you are interested in merging it in I will of course port it to 5.4 branch. I don't see particular reasons to apply it to 5.3, I was working on it because I'm planning to use it in my production environments. It would be really nice if it could make it in the 5.4 because it contains also some name changes in the ini file variables (with BC of course). The biggest and most interesting change is for sure the introduction of a [defaults] config section. More about this below, first the complete changelog of my patch: Detailed changelog of the patch: - Renamed options pm.status_path, ping.path, ping.response into a new logical group diagnostics, so they are now respectively: diagnostics.status_path, diagnostics.ping_path, diagnostics.ping_response. - Reordered in a more logical way the pool ini options, from a most-likely-to-be-customized first to the least. Usually when you edit config file you have big concentration on the first few settings, then you go like blah blah defaults defaults and so on, so this kind of order makes sense. - Aligned all the code listings of pool options with the official order to ease auditing. The official order is the one reported in the struct definition in fpm_conf.h, which of course is the same as the php-fpm.conf.in config template - Improved error messages. A better message helps new adopters to get started quickly, at the beginning I was really puzzled in front of some not very helpful messages. - Introduced a new section [defaults] that allows setting default values - Dropped restriction of not setting the same value multiple times, the last one holds - Removed a lot of redundant checks that are logically implied or not really required, without reducing the robustness of the program. - Improved a lot code readability in some parts, plus added some useful comments in the parts that were less easy to understand. - Refactored some functions to be shorter from code lines point of view, while still doing exactly the same function. - Various white space and cosmetic code improvements - Moved macros STR2STR, BOOL2STR, and PM2STR from fpm_conf.h to fpm_conf.c, no reason to make them public since that's the only file using them, so in case we need to change them in the future, there is less risk of breaking something which depends on them. - Fixed a memory leak in fpm_conf_expand_pool_name() (previous dynamically allocated *value was lost) Now about the new [defaults] section of the config file. In the current version if you want to make a decent looking config file you have to do something like that: [global] ...your global stuff... [pool1] user = pool1 listen = ... include = pool_defaults.conf [pool2] user = pool1 listen = ... include = pool_defaults.conf and so on, plus of course you need the external file pool_defaults.conf. With these changes, you can now do something like this: [global] ...your global stuff... [defaults] ...my defaults valid for every pool... pm.max_children = 500 pm.start_servers = 10 [pool1] user = pool1 listen = ... [pool2] user = pool2 listen = ... [pool3] user = pool3 listen = ... pm.start_servers = 20 There is also a small gain in the parsing time, because defaults are propagated in memory instead of parsed multiple times as with the include files solution. I have to admit this argument is quite irrilevant because it's only a startup time overhead, but it's so much more elegant in my eyes. Also by dropping the constraint of setting strings only once, you can override your defaults in the pools, so you can have an access log format for all of them except one. Upcoming changes that would follow from me if you accept this patch: 1) allow '$pool' variable to be used in the [globals], but this requires a bit of restructuring because it needs to be lazy-expanded in post process time instead of parsing time. 2) possibility to include more than one file per section Looking forward to have some feedback from you
Re: [PHP-DEV] [RFC] Object oriented session handlers
Hi,
2011/6/2 Arpad Ray array...@gmail.com:
Hi,
A while ago I submitted a patch to allow session_set_save_handler() to
accept a class, and support the inheritance of the default session
handler's methods.
The RFC has a more detailed description and the current patch:
https://wiki.php.net/rfc/session-oo
Changes since this was last discussed:
- More sanity checking to prevent handlers being called in unexpected states
- ZTS fixes
Any thoughts?
Regards,
Arpad
I like the idea (and voted +1 on it), but I've some consideration to do:
+/* {{{ proto bool SessionHandler::open(string save_path, string session_name)
+ Wraps the old open handler */
+PHP_METHOD(SessionHandler, open)
+{
+ PS_SANITY_CHECK;
+
+ PS(mod_user_is_open) = 1;
+ RETVAL_LONG(PS(default_mod)-s_open(PS(mod_data), PS(save_path),
PS(session_name) TSRMLS_CC));
+}
[..]
+ZEND_BEGIN_ARG_INFO(arginfo_session_class_open, 0)
+ ZEND_ARG_INFO(0, save_path)
+ ZEND_ARG_INFO(0, session_name)
+ZEND_END_ARG_INFO()
This method does not take the save_path and session_name parameter, it
just use the INI entry.
This lead to following behavior:
$x = new SessionHandler
$x-open(1,2,3,4); // param is not used, and no param check at all
It's missing void param check for the close method as well.
Thank you for helping us make PHP better. :)
--
Regards,
Felipe Pena
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Big patch for FPM (config and initialization)
On 07/03/2011 04:50 AM, Giovanni Giacobbi wrote: Detailed changelog of the patch: - Renamed options pm.status_path, ping.path, ping.response into a new logical group diagnostics, so they are now respectively: diagnostics.status_path, diagnostics.ping_path, diagnostics.ping_response. I'm not quite sure renaming config options would help anybody, in my opinion it would do more harm than good. I personally would certainly expect my existing config files to continue to work after upgrade to 5_4. Yes, we're changing '3' to '4' in the version number, but that doesn't mean we can just go ahead and break the config file. Other than that, looks ok to me (assuming you've tested it and does continue to work the way it worked before all those cosmetic changes). -- Wbr, Antony Dovgal --- http://pinba.org - realtime profiling for PHP -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Big patch for FPM (config and initialization)
FPM is still new, and only just getting the experimental flag removed, now is the only time to make any BC breaking cosmitic changes that will lead to less confusion in the long run.. That said - I'm not sure about the diagnostics group name.. Maybe allow for both the old and new naming on 5.4 and remove the old in 5.next...? Kiall On Jul 3, 2011 9:43 p.m., Antony Dovgal t...@daylessday.org wrote: On 07/03/2011 04:50 AM, Giovanni Giacobbi wrote: Detailed changelog of the patch: - Renamed options pm.status_path, ping.path, ping.response into a new logical group diagnostics, so they are now respectively: diagnostics.status_path, diagnostics.ping_path, diagnostics.ping_response. I'm not quite sure renaming config options would help anybody, in my opinion it would do more harm than good. I personally would certainly expect my existing config files to continue to work after upgrade to 5_4. Yes, we're changing '3' to '4' in the version number, but that doesn't mean we can just go ahead and break the config file. Other than that, looks ok to me (assuming you've tested it and does continue to work the way it worked before all those cosmetic changes). -- Wbr, Antony Dovgal --- http://pinba.org - realtime profiling for PHP -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Big patch for FPM (config and initialization)
+1 the way for change is to add the new, add an E_DEPRECATED style warning for a point release or two, and then nix it. On Sun, Jul 3, 2011 at 2:04 PM, Kiall Mac Innes ki...@managedit.ie wrote: FPM is still new, and only just getting the experimental flag removed, now is the only time to make any BC breaking cosmitic changes that will lead to less confusion in the long run.. That said - I'm not sure about the diagnostics group name.. Maybe allow for both the old and new naming on 5.4 and remove the old in 5.next...? Kiall On Jul 3, 2011 9:43 p.m., Antony Dovgal t...@daylessday.org wrote: On 07/03/2011 04:50 AM, Giovanni Giacobbi wrote: Detailed changelog of the patch: - Renamed options pm.status_path, ping.path, ping.response into a new logical group diagnostics, so they are now respectively: diagnostics.status_path, diagnostics.ping_path, diagnostics.ping_response. I'm not quite sure renaming config options would help anybody, in my opinion it would do more harm than good. I personally would certainly expect my existing config files to continue to work after upgrade to 5_4. Yes, we're changing '3' to '4' in the version number, but that doesn't mean we can just go ahead and break the config file. Other than that, looks ok to me (assuming you've tested it and does continue to work the way it worked before all those cosmetic changes). -- Wbr, Antony Dovgal --- http://pinba.org - realtime profiling for PHP -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Big patch for FPM (config and initialization)
On Sun, Jul 3, 2011 at 22:42, Antony Dovgal t...@daylessday.org wrote:
On 07/03/2011 04:50 AM, Giovanni Giacobbi wrote:
Detailed changelog of the patch:
- Renamed options pm.status_path, ping.path, ping.response into
a new logical group diagnostics, so they are now respectively:
diagnostics.status_path, diagnostics.ping_path,
diagnostics.ping_response.
I'm not quite sure renaming config options would help anybody, in my opinion
it would do more harm than good.
I personally would certainly expect my existing config files to continue to
work after upgrade to 5_4.
Yes, we're changing '3' to '4' in the version number, but that doesn't mean
we can just go ahead and break the config file.
In fact I don't break them, probably I should've specified it more
clearly, but the previous names are still well accepted:
+ /* Backward compatibility options, to be removed in the next major
release */
+ { ping.path, fpm_conf_set_string,
WPO(diag_ping_path) },
+ { ping.response, fpm_conf_set_string,
WPO(diag_ping_response) },
+ { pm.status_path, fpm_conf_set_string,
WPO(diag_status_path) },
+
I can also add a deprecated warning at boot time if you wish. Anyway,
the patch is too big, I admit, if you don't want to accept the change
name I can create another patch without it. But the first time I read
the config file I had a strong feeling that pm.status_path and
ping_path were related that's why i set them to:
/somesecretname/fpm-status
/somesecretname/fpm-ping
Aren't you doing something like that too?
About the testing, well it works, but I didn't test it thoroughly.
Actually I already found a couple of bugs. Who is the official
maintainer of the FPM? If he is OK to merge it then I will do a very
careful test and I will also write some test cases for the config file
parsing (not sure if there is a test suite for that already, didn't
check carefully).
Thank you for your positive feedback :)
--
Giovanni Giacobbi
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
