Problem/Motivation
PHP 5.3 is more stringent with warnings - it will warn about deprecated functions in a lot of default configurations (e.g. Debian, Ubuntu). Drupal 6 currently generates hundreds of messages like this:
Deprecated: Function ereg() is deprecated in /path/to/httpd.www/drupal-6.19/includes/file.inc on line 938
In the install, at least, there is 172 such messages, but since this is used in a loop in file_scan_directory(), it can mean a lot more warnings in certain sites operations. This can flood the DB log, causing higher memory usage and database space.
This can also break the installer from the user's point of view as the Location headers cannot be sent:
Warning: Cannot modify header information - headers already sent by (output started at /path/to/httpd.www/drupal-6.19/includes/file.inc:938) in /path/to/httpd.www/drupal-6.19/includes/install.inc on line 618
Proposed resolution
The current patch simply silences warnings emanating from ereg() calls. Since ereg and preg patterns are different, it is not possible to simply change to using preg, as this would break the API.
A workaround is to disable E_DEPRECATED warnings, but this is not necessarily possible everywhere.
Remaining tasks
None, the patch is RTBC. It's been running in production on Koumbit's servers for a long time, and is strongly recommended by the Aegir team for any deployment, or for anybody using Drush, as some versions of drush disregard error_reporting() policies set by the administrator.
User interface changes
None.
API changes
None.
Original report by Shutterfreak
Apparently the PHP5.3 problems are not solved yet in Drupal 6.19. They make it even impossible to install a fresh Drupal 6.19.
I just uploaded a clean test Drupal 6.19 to my PHP 5.3.3 site, and after running install.php all I got was a ton (172 to be precise) of identical PHP deprecated messages:
Deprecated: Function ereg() is deprecated in /path/to/httpd.www/drupal-6.19/includes/file.inc on line 938
At the end I get the following 2 lines of PHP error messages:
Warning: Cannot modify header information - headers already sent by (output started at /path/to/httpd.www/drupal-6.19/includes/file.inc:938) in /path/to/httpd.www/drupal-6.19/includes/install.inc on line 618
Warning: Cannot modify header information - headers already sent by (output started at /path/to/httpd.www/drupal-6.19/includes/file.inc:938) in /path/to/httpd.www/drupal-6.19/includes/install.inc on line 619
The 2 offending lines in includes/install.inc are both header()
lines in the method below:
function install_goto($path) {
global $base_url;
header('Location: '. $base_url .'/'. $path);
header('Cache-Control: no-cache'); // Not a permanent redirect.
exit();
}
The offending line in includes/file.inc is:
elseif ($depth >= $min_depth && ereg($mask, $file)) {
Comments
Comment #1
ShutterFreak CreditAttribution: ShutterFreak commentedI fixed this problem by replacing line 938 in includes/file.inc with:
Is it a reasonable assumption to say that a pound sigh '#' is an invalid character in a path?
See attached patch. Please review.
Comment #2
ShutterFreak CreditAttribution: ShutterFreak commentedLooking more closely at this issue, I wonder whether we really need a regular expression here for checking for
$mask
. Wouldn't astrstr()
call suffice (and BTW be faster)?Comment #3
Damien Tournoud CreditAttribution: Damien Tournoud commentedCannot reproduce on PHP 5.3.3, even with error_reporting = E_ALL in the global PHP configuration.
Comment #4
Damien Tournoud CreditAttribution: Damien Tournoud commented@ShutterFreak: we are not removing this call to ereg(), as it would be an API change, and we don't do that in stable versions. We have everything in place to disable PHP from reporting E_DEPRECATED errors, and I just confirmed that it works as expected on my system. Not sure what happens for you.
Comment #5
ShutterFreak CreditAttribution: ShutterFreak commented@Damien: as I said I just did a fresh install and I could not get Drupal running unless I patched that ereg() statement in file.inc.
I attached my phpinfo dump to this message in the hope this will help.
Not sure if this bit is important here, but I have no access to php.ini, nor does my .htaccess permissions allow me to use Options directives on that box.
Comment #6
Damien Tournoud CreditAttribution: Damien Tournoud commentedThis phpinfo() extract says that you have error_reporting set to E_ALL. This should not matter because we are removing the E_DEPRECATED bit from the error_reporting array at the beginning on
includes/bootstrap.inc
:I don't know why it doesn't work for you. Without more information or access to this box, it's rather difficult to tell.
Comment #7
ShutterFreak CreditAttribution: ShutterFreak commentedThis either means 3 things:
1. bootstrap.inc is processed AFTER the error occurs. Technically this is possible since
install.php
first includesincludes/install.inc
(line 4).2. My box does NOT get into the
if (defined('E_DEPRECATED')) { ... }
block.3. The functions that were loaded before disabling E_DEPRECATED error messages are not updated and keep spawning E_DEPRECATED messages.
Which makes me wonder why the
require_once './includes/bootstrap.inc';
line (line 20 in install.php) is not the first include?Comment #8
Damien Tournoud CreditAttribution: Damien Tournoud commentedWell. install.php is clearly not the prettiest beast, but the first thing it does (except defining a bunch of functions) is to call install_main() which require_once './includes/bootstrap.inc'. So I doubt about either 1 or 3.
Comment #9
ShutterFreak CreditAttribution: ShutterFreak commentedI'm not sure.
Reading install.php line by line tells me this:
Step 1: include "includes/install.inc"
Step 2: define a parameter and a bunch of functions
Step 3: Invoke install_main()
→ Only in step 3 we include "includes/bootstrap.inc"
A lot might have happened before including bootstrap.inc…
Comment #10
ShutterFreak CreditAttribution: ShutterFreak commentedI redid a clean install on that box, but reverting the ereg() edit, and moving
include_once './includes/bootstrap.inc'
to the 1st executable line of install.php.Unfortunately to no avail.
Are there situations where the PHP
error_reporting()
function is not functioning?Comment #11
mikeytown2 CreditAttribution: mikeytown2 commentedI get this error as well & its quite annoying if using XHProf. Encountered it while enabling a module. I run php showing all errors (this is a test box).
php-fpm 5.3.3 nginx
Comment #12
pillarsdotnet CreditAttribution: pillarsdotnet commented@Damien -- How is the patch in #1 an API change?
EDIT: nevermind; I read the answer in [#http://drupal.org/node/360605]
Comment #13
pillarsdotnet CreditAttribution: pillarsdotnet commentedDuplicate of #360605: PHP 5.3 CompatibilityEDIT: nevermind. (sigh)
Comment #14
pillarsdotnet CreditAttribution: pillarsdotnet commentedI see the other issue is locked and recommends opening up other threads for 6.x continuations.
Comment #15
pillarsdotnet CreditAttribution: pillarsdotnet commentedDoes this work for you?
Comment #16
anarcat CreditAttribution: anarcat commentedThere is at least one circumstance where this happens - drush and aegir. They do their own funky bootstrap and error handling, and they do not disable DEPRECATED warnings.
In fact, I don't think that is a good practice in general. In production, yes, maybe, but in development, you actually want those warnings. Now I understand the argument that we may not be supposed to dev in D6, but that's another debate. ;)
Issue in aegir: #1075322: Creating a site gives many "Function ereg() is deprecated file.inc:941" errors and a broken site
Comment #17
pillarsdotnet CreditAttribution: pillarsdotnet commented@anarcat: you didn't say whether the patch in #15 works for you. I, for one, would be interested in knowing.
Comment #18
anarcat CreditAttribution: anarcat commentedYes, it fixes the issue for me. Here's a patch for you. Not sure it's the right way though - I would rather see a real preg there... but that would change the API and would therefore not be accepted in D6.
Comment #20
pillarsdotnet CreditAttribution: pillarsdotnet commentedPatch suppresses warnings on all ereg functions still in 6.x core.
Comment #21
anarcat CreditAttribution: anarcat commentedThere seem to be unrelated whitespace changes in the patch.
Comment #22
pillarsdotnet CreditAttribution: pillarsdotnet commentedYeah; sorry about that. The emacs drupal.el auto-corrects certain whitespace errors without telling me about it.
Re-rolled without the gratuitous corrections.
Comment #23
pillarsdotnet CreditAttribution: pillarsdotnet commentedGrr... missed one.
Comment #24
kingandy CreditAttribution: kingandy commentedLine numbers are different (possibly because I'm on 6.20, possibly because I'm using Pressflow?), but those changes worked for me.
Comment #25
pillarsdotnet CreditAttribution: pillarsdotnet commentedThere are no ereg calls in a recent git checkout of Drupal 6.x. In fact, the last one was removed in September of 2008:
Comment #26
kingandy CreditAttribution: kingandy commented@Pillars, the file you link to contains calls to both ereg and ereg_replace in includes/file.inc. Both calls are still present in the current 6.22 download from the main project page.
Ereg was indeed removed around 2008-2009, but got put back in because it caused a change in file_scan_directory behaviour, which was considered an unacceptable API change at that stage in Drupal 6's life cycle.
Comment #27
pillarsdotnet CreditAttribution: pillarsdotnet commentedSorry; something must have been wrong with my local clone. Had to delete and re-checkout the 6.x branch to fix.
Patch.
Comment #28
anarcat CreditAttribution: anarcat commentedfor those of us poor bastards still stuck at drush make 2.2 (or that don't have much of a clue about how to apply too much patches) that want to apply this patch to core, here's another patch that will apply cleanly without -p1.
i have also cleaned up the whitespace changes out of the patch.
we are going to start running this in production at koumbit, so you can consider this RTBC now.
Comment #29
j0nathan CreditAttribution: j0nathan commentedSubscribing.
Comment #30
TravisCarden CreditAttribution: TravisCarden commentedComment #31
TravisCarden CreditAttribution: TravisCarden commentedSince
ereg()
has been deprecated along with the rest of the POSIX Regex Functions, doesn't it make more sense to replace it and them with their PCRE replacements instead of just silencing error reporting? Surely that wouldn't be considered an API change, since it wouldn't affect the names or expected input or output of any functions.Comment #32
pillarsdotnet CreditAttribution: pillarsdotnet commented@#31 by TravisCarden on October 7, 2011 at 8:46pm:
This is a frequently-unanswered question (FUQ).
See #360605-36: PHP 5.3 Compatibility
Briefly, it would affect the expected input/output of those functions which specifically state in their API that an ereg-compatible regex is expected.
Comment #33
kingandy CreditAttribution: kingandy commentedFor what it's worth, the ereg has been changed to preg in later Drupal releases (7.x +). Suppressing the warnings is the appropriate measure at this stage of Drupal 6.x's life cycle.
Comment #34
anarcat CreditAttribution: anarcat commentedNo, this *is* an API change as the $mask variable changes meaning and syntax between preg and ereg. Please let's get this silly issue over with already...
Comment #35
TravisCarden CreditAttribution: TravisCarden commentedSatisfies me! Thank you.
Comment #36
pillarsdotnet CreditAttribution: pillarsdotnet commentedSomebody should write an issue summary. Also tagging "Novice" in a fit of wild optimism.
Comment #37
ShutterFreak CreditAttribution: ShutterFreak commentedWell, since the ereg calls won't be replaced any time soon with preg calls, I prefixed the deprecated methods with an @ sign to make things work. This is an important item to keep in mind when installing Drupal on PHP 5.3.
Comment #37.0
anarcat CreditAttribution: anarcat commentedUpdated issue summary.
Comment #38
anarcat CreditAttribution: anarcat commented@pillarsdotnet - summary written, even if I'm not a novice.
Nothing seems to be blocking this now. Can we fix this critical already? The patch is in #28.
Comment #39
kattekrab CreditAttribution: kattekrab commented... ok I will have a go at writing up an issue summary!
Comment #40
kattekrab CreditAttribution: kattekrab commentedoops - now I'm reading the issue summary... so someone's already done that it seems.
Comment #41
Dane Powell CreditAttribution: Dane Powell commentedI can also confirm the patch works, let's commit it...
Comment #42
kattekrab CreditAttribution: kattekrab commentedtumbleweeds
Comment #43
theohawse CreditAttribution: theohawse commented#27: ereg-suppress_warnings-883038-27.patch queued for re-testing.
Comment #44
Gábor HojtsyCommitted this to Drupal 6, thank you! I agree without changing the API there is not much of a better way to get rid of these warnings, so this is our best shot for Drupal 6.
Comment #45
theohawse CreditAttribution: theohawse commentedhttp://drupalcode.org/project/drupal.git/commit/ae71f5d
Heck yeah open source in action! Can't believe this made it to core :)
Comment #46
kattekrab CreditAttribution: kattekrab commentedYay! Thanks for doing the commit @Gábor Hojtsy
Comment #47.0
(not verified) CreditAttribution: commentedUpdated issue summary.