New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug #46408: Fix double formatting for PostgreSQL bound parameters #186

Closed
wants to merge 1 commit into
from

Conversation

5 participants
@asmecher

asmecher commented Sep 6, 2012

See https://bugs.php.net/bug.php?id=46408 for extensive discussion of this.

@reeze

View changes

ext/pgsql/pgsql.c
@@ -1736,7 +1737,15 @@ static void _php_pgsql_free_params(char **params, int num_params)
} else {
zval tmp_val = **tmp;
zval_copy_ctor(&tmp_val);
+
+ // PSQL requires . for radix; convert to string,

This comment has been minimized.

@reeze

reeze Sep 9, 2012

Contributor

please use '/* */' instead of '//'

@reeze

reeze Sep 9, 2012

Contributor

please use '/* */' instead of '//'

This comment has been minimized.

@lstrojny

This comment has been minimized.

Show comment
Hide comment
@lstrojny

lstrojny Sep 13, 2012

Contributor

While this fix sure does what it needs to do to address this problem, I’m not sure we should work around this design flaw on every single occasion. Additionally the bugfix misses a test to make sure we don’t get a regression at some point.

Contributor

lstrojny commented Sep 13, 2012

While this fix sure does what it needs to do to address this problem, I’m not sure we should work around this design flaw on every single occasion. Additionally the bugfix misses a test to make sure we don’t get a regression at some point.

@lstrojny

View changes

ext/pgsql/pgsql.c
+
+ /* PSQL requires . for radix; convert to string, avoiding problems with doubles
+ and locales using , as a radix character instead (see bug #46408) */
+ char *current_locale = setlocale(LC_NUMERIC, "C");

This comment has been minimized.

@lstrojny

lstrojny Sep 13, 2012

Contributor

As we use C89/C90, declarations may not be mixed with code. Please move the declaration to the beginning of the block.

@lstrojny

lstrojny Sep 13, 2012

Contributor

As we use C89/C90, declarations may not be mixed with code. Please move the declaration to the beginning of the block.

This comment has been minimized.

@lstrojny

lstrojny Sep 13, 2012

Contributor

Additionally: are you certain this works on any platform we support?

@lstrojny

lstrojny Sep 13, 2012

Contributor

Additionally: are you certain this works on any platform we support?

@asmecher

This comment has been minimized.

Show comment
Hide comment
@asmecher

asmecher Sep 13, 2012

Thanks for the hints, Lars -- I've moved the var declaration to the head of the block, added #ifdef checks to make sure HAVE_SETLOCALE is respected, and added a regression test (ext/pgsql/tests/bug46408.phpt). I haven't contributed code to PHP before so your scrutiny is appreciated.

Thanks for the hints, Lars -- I've moved the var declaration to the head of the block, added #ifdef checks to make sure HAVE_SETLOCALE is respected, and added a regression test (ext/pgsql/tests/bug46408.phpt). I haven't contributed code to PHP before so your scrutiny is appreciated.

@asmecher

This comment has been minimized.

Show comment
Hide comment
@asmecher

asmecher Sep 13, 2012

As for whether or not this problem should be worked around in PHP, it's necessary either to do it there or to do it in my own PHP scripts (and for everyone else who needs both setlocale and pg_query_params). I agree that it's not ideal but it does rehabilitate that function; I wouldn't know what to suggest as a broader strategy.

Note that the test will require the hr_HR locale to be generated on the test system (see /etc/locale.gen).

As for whether or not this problem should be worked around in PHP, it's necessary either to do it there or to do it in my own PHP scripts (and for everyone else who needs both setlocale and pg_query_params). I agree that it's not ideal but it does rehabilitate that function; I wouldn't know what to suggest as a broader strategy.

Note that the test will require the hr_HR locale to be generated on the test system (see /etc/locale.gen).

@lstrojny

This comment has been minimized.

Show comment
Hide comment
@lstrojny

lstrojny Sep 18, 2012

Contributor

Alright, digged a little deeper into this issue and here is my first analysis. The code flow basically goes like this

#define convert_to_string(zval *op) (defined in Zend/zend_operators.h)
    void _convert_to_string(zval *op ZEND_FILE_LINE_DC)  (defined in Zend/zend_operators.c)
        zend_spprintf(&Z_STRVAL_P(op), 0, "%.*G", (int) EG(precision), dval) (which is assigned in Zend/zend.c)
            zend_vspprintf = utility_functions->vspprintf_function (which is actually assigned in main/main.c)
                zuf.vspprintf_function = vspprintf (which is defined in main/spprintf.c)
                    PHPAPI int vspprintf(char **pbuf, size_t max_len, const char *format, va_list ap) (defined in main/spprintf.c)
                        static void xbuf_format_converter(smart_str *xbuf, const char *fmt, va_list ap) (defined in main/spprintf.c)

case 'G' defines the behavior for %.*G. If I read the code correctly, '%.*H' wouldn’t be locale sensitive.

This means: we need something like _convert_to_string_but_ignore_locale_settings() (no, this is not a serious function name proposal) which does the same as _convert_to_string() but uses '%.*H' for floats.

Contributor

lstrojny commented Sep 18, 2012

Alright, digged a little deeper into this issue and here is my first analysis. The code flow basically goes like this

#define convert_to_string(zval *op) (defined in Zend/zend_operators.h)
    void _convert_to_string(zval *op ZEND_FILE_LINE_DC)  (defined in Zend/zend_operators.c)
        zend_spprintf(&Z_STRVAL_P(op), 0, "%.*G", (int) EG(precision), dval) (which is assigned in Zend/zend.c)
            zend_vspprintf = utility_functions->vspprintf_function (which is actually assigned in main/main.c)
                zuf.vspprintf_function = vspprintf (which is defined in main/spprintf.c)
                    PHPAPI int vspprintf(char **pbuf, size_t max_len, const char *format, va_list ap) (defined in main/spprintf.c)
                        static void xbuf_format_converter(smart_str *xbuf, const char *fmt, va_list ap) (defined in main/spprintf.c)

case 'G' defines the behavior for %.*G. If I read the code correctly, '%.*H' wouldn’t be locale sensitive.

This means: we need something like _convert_to_string_but_ignore_locale_settings() (no, this is not a serious function name proposal) which does the same as _convert_to_string() but uses '%.*H' for floats.

@asmecher

This comment has been minimized.

Show comment
Hide comment
@asmecher

asmecher Sep 19, 2012

Thanks for the feedback, Lars -- I've introduced convert_to_string_unlocalized and used it instead. I've attempted to keep code duplication to a minimum -- personally, I would have preferred to introduce an optional parameter to the existing _convert_to_string but I suspect that would not follow PHP coding conventions.

Thanks for the feedback, Lars -- I've introduced convert_to_string_unlocalized and used it instead. I've attempted to keep code duplication to a minimum -- personally, I would have preferred to introduce an optional parameter to the existing _convert_to_string but I suspect that would not follow PHP coding conventions.

@lstrojny

This comment has been minimized.

Show comment
Hide comment
@lstrojny

lstrojny Sep 19, 2012

Contributor

What about convert_to_cstring (like in c locale?)

Btw: regarding locale, you could also try to iterate over a few variants (de_DE e.g. also uses "," as a decimal separator).

Contributor

lstrojny commented Sep 19, 2012

What about convert_to_cstring (like in c locale?)

Btw: regarding locale, you could also try to iterate over a few variants (de_DE e.g. also uses "," as a decimal separator).

@asmecher

This comment has been minimized.

Show comment
Hide comment
@asmecher

asmecher Sep 19, 2012

Agreed; changed.

Agreed; changed.

@lstrojny

This comment has been minimized.

Show comment
Hide comment
@lstrojny

lstrojny Sep 19, 2012

Contributor

Just so that you know, I've written a mail to internals@php.net to get more people into this discussion: http://marc.info/?l=php-internals&m=134807980716416&w=2

Contributor

lstrojny commented Sep 19, 2012

Just so that you know, I've written a mail to internals@php.net to get more people into this discussion: http://marc.info/?l=php-internals&m=134807980716416&w=2

@asmecher

This comment has been minimized.

Show comment
Hide comment
@asmecher

asmecher Sep 19, 2012

Thanks, Lars. This has been helpful and informative for me and I hope it results in an accepted fix.

Thanks, Lars. This has been helpful and informative for me and I hope it results in an accepted fix.

@asmecher

This comment has been minimized.

Show comment
Hide comment
@asmecher

asmecher Oct 3, 2012

Unfortunately that round on the php-internals list wasn't too encouraging. What would you suggest for next steps? I'd like to see a general fix applied, but the idea didn't seem popular. Alternately, I could alter this to just affect pg_query_params, but that won't help maintainability going forward.

asmecher commented Oct 3, 2012

Unfortunately that round on the php-internals list wasn't too encouraging. What would you suggest for next steps? I'd like to see a general fix applied, but the idea didn't seem popular. Alternately, I could alter this to just affect pg_query_params, but that won't help maintainability going forward.

@smalyshev

View changes

ext/pgsql/pgsql.c
@@ -23,6 +23,7 @@
/* $Id$ */
#include <stdlib.h>
+#include <locale.h>

This comment has been minimized.

@smalyshev

smalyshev Oct 27, 2012

Contributor

why you need locale.h?

@smalyshev

smalyshev Oct 27, 2012

Contributor

why you need locale.h?

This comment has been minimized.

@lstrojny

lstrojny Oct 27, 2012

Contributor

Yep, that's no longer needed.

@lstrojny

lstrojny Oct 27, 2012

Contributor

Yep, that's no longer needed.

@lstrojny

This comment has been minimized.

Show comment
Hide comment
@lstrojny

lstrojny Jan 14, 2013

Contributor

Could you squash the commits. I will apply your fix for the PHP 5.5 branch.

Contributor

lstrojny commented Jan 14, 2013

Could you squash the commits. I will apply your fix for the PHP 5.5 branch.

@asmecher

This comment has been minimized.

Show comment
Hide comment
@asmecher

asmecher Jan 14, 2013

Done. Thanks for all your help, Lars.

Done. Thanks for all your help, Lars.

@php-pulls

This comment has been minimized.

Show comment
Hide comment
@php-pulls

php-pulls Jan 14, 2013

Comment on behalf of lstrojny at php.net:

Merged into 5.5 and master. Thanks for your contribution and your patience!

Comment on behalf of lstrojny at php.net:

Merged into 5.5 and master. Thanks for your contribution and your patience!

@php-pulls php-pulls closed this Jan 14, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment