Move replace fix_gpc_magic() with a hard requirement for magic quotes to be disabled

I am fully in support of option 4. magic_quotes have been disabled by default in PHP since before any of us were even using Drupal. It's explicitly deprecated in 5.3. It doesn't even exist in 5.4, and by the time D8 ships PHP 5.3 will be on long-term-security-fix-only mode it will be so old.

Plus our htaccess file already disables it. You really have to go out of your way to have magic_quotes on. Let's just cut to the chase and either ignore it entirely or hard-fail.

Follow-up: I believe we already outright refuse to run if register_globals are enabled. magic_quotes are in the same family of PHP 3-era dumb.

+1 to moving unicode_check() earlier and removing fix_gpc_magic().

+++ b/core/includes/bootstrap.inc
@@ -2164,6 +2169,11 @@ function _drupal_bootstrap_configuration() {
+
+  // Detect string handling method.
+  require_once DRUPAL_ROOT . '/core/includes/unicode.inc';
+  unicode_check();
+

Any reason not to move this into drupal_environment_initialize() which already ends with a setlocale() function, so this would logically follow? Only potential downside to that that I can think of is that it would then happen before the page timer starts, but I also wonder if we could avoid including unicode.inc this early and instead move unicode_check() to bootstrap.inc. Also, we can pass a parameter to _unicode_check() (which is already incorrectly documented as taking one) to avoid running t() when at this calling point, we don't use the result anyway. At which point, there's nothing time consuming happening here, so it's okay for it to happen before the page timer starts.

As per #1541990-9: Improve check before calling t() (D8 drupal_container() patch broke drush), this will currently fatal error if t() is called from a failing _unicode_check() condition, because t() fatal errors if called before drupal_classloader() (which itself depends on DRUPAL_BOOTSTRAP_CONFIGURATION). Moshe asked in that other issue if we can make t() or drupal_container() robust enough to not fatal error when called before we have a class loader. We can, but I don't know if we should. If Drupal 8 is increasingly moving to autoloaded classes, I don't know if we want to start having lots of procedural wrappers containing code that branches on whether we have an autoloader.

For this issue, the return values from t() aren't used anyway (from this particular check at this particular point in bootstrap), so we can sidestep the problem by having unicode_check() pass _unicode_check() a parameter to not return error strings. But, maybe this is a good issue in which to discuss whether t() and other bootstrap functions that use classes should be made to worry about the possibility of running before there's an autoloader.

IMO, anything that happens before the class loader is enabled is caveat emptor. Nothing but hard-coded core initialization code should do so, and we should keep that to an absolute minimum. Currently install.php, update.php, etc. violate that because they're one off dedicated scripts. Fixing that is a separate issue.

So no, we should not special case things for when the class loader doesn't exist yet. We should just make sure nothing non-trivial happens before there is a class loader.

For this issue, the return values from t() aren't used anyway (from this particular check at this particular point in bootstrap), so we can sidestep the problem by having unicode_check() pass _unicode_check() a parameter to not return error strings.

Just for clarity, here's what I mean.

re to @sun in #15: my request is not for t() to actually work very early in the bootstrap. My request is for it to return an untranslated string instead of a fatal error. Thats what we finally accomplished in D7, and Caeveat Emptor regresses us back to darker days.

Edit: I'm suggesting that t() add a try/catch and then catch an exception when language system is not available and return untranslated string in that case.

IMO, anything that happens before the class loader is enabled is caveat emptor.

If we agree with this, we can help make it clear with #1593826: Split bootstrap.inc into functions available before/by settings.php and functions available after there's a class loader.

Per the OP, what about get_magic_quotes_gpc() and get_magic_quotes_runtime()?

I like #19.

+++ b/core/includes/bootstrap.inc
@@ -577,6 +582,10 @@ function drupal_environment_initialize() {
+  require_once DRUPAL_ROOT . '/core/includes/unicode.inc';

This patch moves the 3 constants and 2 functions into bootstrap.inc rather than requiring the entire file to be loaded for cached pages.

+++ b/core/includes/unicode.inc
@@ -141,14 +142,33 @@ function unicode_requirements() {
+    case 'mbstring.func_overload':
+      $requirements['unicode']['description'] = $t('Multibyte string function overloading in PHP is active and must be disabled. Check the php.ini <em>mbstring.func_overload</em> setting. Please refer to the <a href="@url">PHP mbstring documentation</a> for more information.', array('@url' => 'http://www.php.net/mbstring'));

This patch changes this one to use $t_args to match the others.

With the patch this time.

I like it. At first, I was concerned about #23 introducing the side-effect of setting the global as a result of calling unicode_requirements(), but it's already the case that doing that had the side-effect of possibly calling mb_internal_encoding() and mb_language() so #23 doesn't create a new side-effect, but makes the existing one consistent. A future issue can deal with removing the side-effect entirely if we want better unit testability of unicode_requirements() (which is already challenged, however, by the ini_get() calls), but that's no reason to hold up this issue, so RTBC.

I'm not convinced its not creating a new side effect. Its exacerbating a side effect by adding a global for the system to interact with which makes things less testable. Is there actually a reason we're doing it because I don't see the point.

#23 doesn't change what unicode_check() does with respect to the global, but it does change what unicode_requirements() does with respect to it. However, as per #24, I think that's okay, because even in HEAD, unicode_requirements() has the side effects mentioned, so I actually think it's a current bug for it to have that without also setting the global for consistency. @neclimdul, please RTBC or say if you still disagree. If this is a sticking point, we can go back to #21, and work through this point in a follow-up.

Gotcha, thanks for clearing that up. That was my only concern so lets do it.

Yeah this looks good to me, and while the global's not ideal, I agree a conditionally defined constant just to avoid setting the global would be worse. I don't see another solution coming through without a tonne of refactoring.

Committed/pushed to 8.x.

We should add a change notice and/or update http://drupal.org/requirements for this change in addition to the CHANGELOG.txt note.

I added a change notice at http://drupal.org/node/1600010 for both this issue and #576508: Require PHP 5.3, which can hopefully act as a general placeholder for such info that can be moved to http://drupal.org/requirements when the time comes.

Also, this change broke drush in stock MAMP. :( Sad panda.

[Edited to clarify what's broke]

Details please? I'm running stock MAMP and do not see anything noticeably broken.

Sorry, that was lazy.

When you attempt to use drush, you'll get:

$ drush en statsitics
PHP's 'magic_quotes_gpc' and 'magic_quotes_runtime' settings are not supported and must be disabled.Drush command terminated abnormally due to an unrecoverable error.   [error]

It's confusing because you can install from the UI just fine. Presumably because that's got Drupal's .htaccess or whatever forcing it to false.

It's also confusing because the only reference in MAMP's php.ini file is to magic_quotes_sybase or something and it's set to Off. There is no just plain "magic_quotes_gpc" variable. However, if you run php -i | grep magic from the command-line, you'll see that it does have it set, and set to On. To fix, you'll need to edit your php.ini file, manually add an entry and set it to Off.

You really have to go out of your way to have magic_quotes on.

I can confirm #34, so this assumption was wrong. Running drush in stock MAMP is not exactly going out of your way.

From http://www.php.net/manual/en/security.magicquotes.what.php:

This feature has been DEPRECATED as of PHP 5.3.0 and REMOVED as of PHP 5.4.0.

magic_quotes_gpc Affects HTTP Request data (GET, POST, and COOKIE). Cannot be set at runtime, and defaults to on in PHP.

If I'm reading those two statements correctly, then it's not just MAMP, but that PHP 5.3 in general has a deprecated feature enabled by default. Lovely.

Wait a minute, let me make sure I understand.

Any sane host or distribution has, for a decade, had magic quotes off, or made it possible to turn off.

Magic quotes no longer exists in the current stable version of PHP. Its imminent demise has been known for years.

We have an .htaccess file that turns magic quotes off, assuming .htaccess files are parsed.

... and yet MAMP still has it on? Even with the .htaccess file? Meaning MAMP is both misconfigured by default and doesn't let us configure it safely?

Am I understanding this right? If so, someone needs to explain to me why we don't just file a bug upstream in MAMP and tell them to fix their default configuration, because it is already broken. We already refuse to run period for register_globals. If MAMP can't default to a sane configuration, that's their problem, IMO.

@Crell:

... and yet MAMP still has it on? Even with the .htaccess file?

webchick said:

When you attempt to use drush, you'll get:

(emphasis added).

Upstream MAMP bug along with a http://drupal.org/requirements update seems enough for me though yes.

I agree with last few posts. Not a drupal or drush issue. Back to fixed.

This is just a case of PHP insanity. At least on PHP 5.3.8 (shipped with OS X 10.6), magic_quotes_gpc is deprecated, but it defaults to "on" when PHP has no configuration file :)

php -r 'echo ini_get("magic_quotes_gpc");'
> 1

Not sure if it got fixed later on, but we are going to need to workaround this one way or another.

Moving back to active.

I had a quick look on google, there are several "how do I disable this on MAMP?" results but couldn't find a bug report against MAMP for this.

Seems worth a PHP bug report as well, would be good if they eventually fix the default in 5.3.x.

If we've filed those two, I think that's the best we can do and we should move this back to fixed.

I'm really not keen on reverting the patch, disabling it in php.ini isn't hard if you have control over your webserver. And if you don't have control and it's set to on, then it's time to get a new host.

I think people have seen that with MAMP because they are probably using the default PHP binary from the OS X system.

I think Drush is going to have to workaround this in its wrapper script.

Nope. Hopefully someone else can.

To get drush to work, I added lines for runtime and gpc to explicitly turn them off. And found http://mediatribe.net/en/node/61#comment-808 helpful.

; Magic quotes
;

; Use Sybase-style magic quotes (escape ' with '' instead of \').
magic_quotes_sybase = Off
magic_quotes_runtime = Off
magic_quotes_gpc = Off

http://drupal.org/requirements says to do as much. But I didn't think to look there until I had googled the drush error

> drush status
PHP's 'magic_quotes_gpc' and 'magic_quotes_runtime' settings are not supported and must be disabled.Drush command terminated abnormally due to an unrecoverable [error]
error.

and found this issue and read through it. The site installed ok, and I did not run into the error until I tried to run drush status on the command line. That might lead me to look in the drush readme file, but it does not mention the magic settings.

I have reported an issue on the MAMP bug tracker. The tracker requires registration, and given the activity level I don't hold out much hope of it being addressed quickly. :/

I had same problem in MAMP, edited php.ini and restarted server, but magic_quotes_sybase had old value.
After investigation found that it uses different php.ini for command line:
echo "<?php phpinfo();" | php

Configuration File (php.ini) Path => /etc
Loaded Configuration File => (none)

So what I did is just copied edited php.ini to /etc/ like this:
sudo cp /Applications/MAMP/bin/php/php5.3.14/conf/php.ini /etc/php.ini
Then after restart magic_quotes_sybase become OFF in command line as well and drush works!
Hope that help someone.