default.settings.php contains a number of ini_set() calls. I have been trying to find out the purpose of each of these settings, but without much luck. The comment above doesn't indicate, whether they are required, recommended, suggested or just shown for illustration purposes.

AFAICT from browsing the source code of PHP itself, session.cache_expire is only used, when session.cache_limiter is either "public" or "private_no_expire", but session.cache_limiter is set to "none". Does that mean that the setting of session.cache_expire has no effect?
When this setting was initially committed to CVS back in 2001, the commit comment was "tweaked some of the php settings", i.e. there was no reference to an issue that could explain the reasoning behind.

According to the PHP manual, url_rewriter.tags is only used when transparent sid support is enabled, but session.use_trans_sid is set to 0. Does that mean that the setting of url_rewriter.tags has no effect?
It looks as if the url_rewriter.tags setting was added because session.use_trans_sid could not be set using ini_set() in some versions of PHP4.

Also, I am not sure about the purpose of arg_separator.output, not only in relation to Drupal but in general. The PHP manual doesn't say much, except that it affects http_build_query(), and that isn't used in Drupal.
When this setting was initially committed to CVS in 2003, the commit comment was "Added 'php_value arg_separator.output "&"' as suggested by Curtis", but again there was no reference to an issue that could explain the reasoning behind.

I hope somebody can shed some light on this. If it turns out that some settings are no longer relevant, I believe they should be removed from the file, but in any case I think default.settings.php will benefit from a brief comment about the consequences of changing the settings.

CommentFileSizeAuthor
#8 session.use_trans_sid.patch804 bytesjohn morahan
#6 ini_set-2.patch4.3 KBc960657
#4 ini_set-1.patch3.66 KBc960657

Comments

c960657’s picture

c960657 commented

A substantial number of the unit tests break if magic_quotes_runtime is enabled. I don't see why anyone would want to turn this on, so I suggest moving this from settings.php to .htaccess where magic_quotes_gpc and others are set.

c960657’s picture

c960657 commented

WRT magic_quotes_runtime, see #308834: Move ini_set('magic_quotes_runtime', 0) out of settings.php.

ainigma32’s picture

ainigma32 commented

Maybe you could write a patch removing the settings you think are/have become redundant?

- Arie

c960657’s picture

c960657 commented
Status: Active » Needs review
StatusFileSize
new ini_set-1.patch3.66 KB

Done :-)

session.save_handler is removed, because this setting is automatically set by session_set_save_handler():
http://cvs.php.net/viewvc.cgi/php-src/ext/session/session.c?annotate=php...

magic_quotes_sybase is removed, because - according to the PHP manual - it is only relevant if magic_quotes_gpc or magic_quotes_runtime are enabled - and they are not (magic_quotes_gpc is disabled in .htaccess, and magic_quotes_runtime is disabled in drupal_initialize_variables()).

session.cache_limiter is moved to drupal_initialize_variables() because Drupal's explicitly sends caching-related headers, so PHP's session handler shouldn't interfere with that. session.cache_expire is removed, because session.cache_limiter is hardcoded to none (see reasoning above).

session.use_only_cookies and session.use_trans_sid are moved to drupal_initialize_variables(), because appearently you cannot even log in the session handler is forced to put the session id the query string. url_rewriter.tags is removed because it is only relevant when session.use_trans_sid is enabled.

arg_separator.output is removed, simply because I don't see any point in specifying it (but I may be missing something).

Status: Needs review » Needs work

The last submitted patch failed testing.

c960657’s picture

c960657 commented
Category: support » task
Status: Needs work » Needs review
StatusFileSize
new ini_set-2.patch4.3 KB

Reroll.

dries’s picture

dries commented
Status: Needs review » Fixed

This looks like a great clean-up. Committed to CVS HEAD. Thanks.

john morahan’s picture

john morahan commented
Status: Fixed » Needs review
StatusFileSize
new session.use_trans_sid.patch804 bytes

I do not think you intended to change session.use_trans_sid to 1.

dries’s picture

dries commented
Status: Needs review » Fixed

Committed to CVS HEAD. Thanks.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.

gpk’s picture

gpk commented

Related: #289120: Add magic_quotes_sybase = 0 to .htaccess (was: ajax features not working from a non-default multi-site site)

Note that at on 9 January a patch was committed from that issue that restored the magic_quotes_sybase setting, but in .htaccess instead at run-time (so it won't always work anyway).

I think there is a small flaw in the logic at #4 above - see http://drupal.org/node/289120#comment-1194798.

c960657’s picture

c960657 commented

gpc, do you have an example of a configuration where the current solution doesn't work? AFAICT it is actually useful that magic_quotes_sybase and magic_quotes_gpc are both set in .htaccess. I haven't been able to reproduce any problems with HEAD (I tried various combinations of the two settings with CGI and Apache module).

If both magic_quotes_gpc and magic_quotes_sybase are enabled in php.ini, and .htaccess is not honoured, and we disable magic_quotes_sybase in drupal_initialize_variables(), then fix_gpc_magic() will not work, because stripslashes()'s behaviour depends on the magic_quotes_sybase setting - so the setting has to be the same as when the magic quotes were added. Does that make sense?

If magic_quotes_gpc is enabled in php.ini, and .htaccess is not honoured, then fix_gpc_magic() is always able to strip the magic quotes – right?

gpk’s picture

gpk commented

>do you have an example of a configuration where the current solution doesn't work?
No I don't! And I must confess I'm not familiar with the detail of how stripslashes()'s behaviour is affected by magic_quotes_sybase (perhaps I should read the docs :-P).

I was working on the assumption that what Drupal used to do (before this issue) was OK, i.e. turn off magic_quotes_sybase via ini_set(). From what you are saying this is not 100% OK?

The only problem I can think of at the moment is that possibly the original problem at #289120: Add magic_quotes_sybase = 0 to .htaccess (was: ajax features not working from a non-default multi-site site) could recur because we could now (for the first time in a few years perhaps!) have Drupal running with magic_quotes_sybase on. The problem this caused in the other issue was that double quotes in JSON were not being escaped, preventing AJAXy stuff from working (update.php, Views..). It happened because of mis-cofinguration of settings.php (the ini_set() of magic_quotes_sybase was not present).

c960657’s picture

c960657 commented

I was working on the assumption that what Drupal used to do (before this issue) was OK, i.e. turn off magic_quotes_sybase via ini_set(). From what you are saying this is not 100% OK?

I don't think it is/was.

I reverted my checkout to a version prior to check-in of ini_set-2.patch (cvs up -D2008-12-22), enabled magic_quotes_sybase and magic_quotes_gpc in php.ini, requested the URL http://example.com/?foo=a'b and var_dump()'ed $_GET before and after fix_gpx_magic(). In both cases, $_GET was array("foo" => "a''b", "q" => "node").

So it looks like that bug was accidentally fixed with this issue :-)

gpk’s picture

gpk commented

>So it looks like that bug was accidentally fixed with this issue :-)
Cool...

...but has it introduced a new bug i.e. the quoting problem with JSON as originaly described in #289120: Add magic_quotes_sybase = 0 to .htaccess (was: ajax features not working from a non-default multi-site site), in the situation where magic_quotes_sybase is on in php.ini and PHP is running as CGI?

c960657’s picture

c960657 commented

I don't quite follow what is going on in #290120. But I think I know what may be going on.

Apparently the magic_quotes_sybase setting is used e.g. by stripslashes() and addslashes(), even though magic_quotes_runtime and magic_quotes_gpc are both disabled – contrary to what is indicated in the section in PHP manual that I linked to in #4. The drupal_parse_info_file() function uses stripslashes() and expect it to work the non-sybase way, so we need to disable magic_quotes_sybase. However, in order for fix_gpc_magic() to work, magic_quotes_sybase should have the original value, i.e. the same value as prior to the invocation of index.php, i.e. the setting specified in php.ini, .htaccess, httpd.conf etc., so we cannot disable magic_quotes_sybase in default.settings.php or drupal_initialize_variables() – at least not without restoring the original value prior to calling stripslashes (apparently the original setting is not the one that is restored with ini_restore()).

To fix this, we have several options, including:

  1. Do not change magic_quotes_sybase in default.settings.php or drupal_initialize_variables(), but disable it in _drupal_bootstrap_full() after the call to fix_gpc_magic(). Any calls to stripslashes() (other than those in the fix_gpc_magic functions) during the bootstrap are not guaranteed to work as expected.
  2. Move the fix_gpc_magic functions to bootstrap.inc and call them from drupal_initialize_variables(), and then disable magic_quotes_sybase using ini_set().
  3. Disable magic_quotes_sybase in drupal_initialize_variables(), but save the initial value of the setting in a global or static variable and restore the setting in fix_gpc_magic(), and then disable the setting again afterwards.

I prefer #3.

gpk’s picture

gpk commented

Ah, sorry, finger trouble, I meant #289120 not #290120 (now corrected above).

The problem was caused by magic_quotes_sybase being On (due to user misconfiguration of settings.php in multisite), and as a result addslashes() misbehaved in drupal_to_js(). In that it didn't add any.

I agree that the PHP docs are confusing and yes I'd agree that it looks like magic_quotes_sybase always does its own thing irrespective of whether overrides _gpc and _runtime are on or off (essentially it turns them off AFAICS).

Agree, 1. is not ideal since the behaviour of add/stripslashes prior to full bootstrap phase will depend on server environment.

2. Has the advantage that $_GET/POST/COOKIE are fixed at the earliest possible opportunity.

3. Has the (performance) advantage that we wait until full bootstrap. The downside is that (as at present) $_GET/POST/COOKIE are not fixed prior to hook_boot() (nor hook_exit() on cached page views). If these do anything with $_GET/POST/COOKIE then behaviour could vary between servers. Also the Drupal path is initialized before $_GET/POST/COOKIE are fixed. Whether any of these points really matter I'm not sure.

Anyway another option - 2.5 say - would be to do the fix_gpc_magic() in the initialize path bootstrap phase.

Also it's worth bearing in mind that it will be fairly unusual I think for magic_quotes_sybase to be on, and if it is on then it will have probably been set thus by the user (I can't imagine this being a default on many shared servers).

And so a final option 4. say could be to make magic_quotes_sybase Off a requirement.

I don't feel qualified to choose between 2, 2.5, 3, 4.