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)) {

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ShutterFreak’s picture

Status: Active » Needs review
FileSize
861 bytes

I fixed this problem by replacing line 938 in includes/file.inc with:

elseif ($depth >= $min_depth && preg_match("#$mask#", $file)) {

Is it a reasonable assumption to say that a pound sigh '#' is an invalid character in a path?

See attached patch. Please review.

ShutterFreak’s picture

Looking more closely at this issue, I wonder whether we really need a regular expression here for checking for $mask. Wouldn't a strstr() call suffice (and BTW be faster)?

Damien Tournoud’s picture

Status: Needs review » Postponed (maintainer needs more info)

Cannot reproduce on PHP 5.3.3, even with error_reporting = E_ALL in the global PHP configuration.

Damien Tournoud’s picture

@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.

ShutterFreak’s picture

FileSize
60.01 KB

@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.

Damien Tournoud’s picture

This 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:

// Hide E_DEPRECATED messages.
if (defined('E_DEPRECATED')) {
  error_reporting(error_reporting() & ~E_DEPRECATED);
}

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.

ShutterFreak’s picture

This either means 3 things:

1. bootstrap.inc is processed AFTER the error occurs. Technically this is possible since install.php first includes includes/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?

Damien Tournoud’s picture

Well. 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.

ShutterFreak’s picture

I'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…

ShutterFreak’s picture

I 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?

mikeytown2’s picture

I 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

pillarsdotnet’s picture

@Damien -- How is the patch in #1 an API change?

EDIT: nevermind; I read the answer in [#http://drupal.org/node/360605]

pillarsdotnet’s picture

Status: Postponed (maintainer needs more info) » Closed (duplicate)

Duplicate of #360605: PHP 5.3 Compatibility

EDIT: nevermind. (sigh)

pillarsdotnet’s picture

Status: Closed (duplicate) » Postponed (maintainer needs more info)

I see the other issue is locked and recommends opening up other threads for 6.x continuations.

pillarsdotnet’s picture

Does this work for you?

diff --git includes/file.inc includes/file.inc
index b576a21..198fcb4 100644
--- includes/file.inc
+++ includes/file.inc
@@ -938,7 +938,7 @@ function file_scan_directory($dir, $mask, $nomask = array('.', '..', 'CVS'), $c
           // Give priority to files in this folder by merging them in after any subdirectory files
           $files = array_merge(file_scan_directory("$dir/$file", $mask, $nomask, $callback, $recur
         }
-        elseif ($depth >= $min_depth && ereg($mask, $file)) {
+        elseif ($depth >= $min_depth && @ereg($mask, $file)) {
           // Always use this match over anything already set in $files with the same $$key.
           $filename = "$dir/$file";
           $basename = basename($file);
anarcat’s picture

Status: Postponed (maintainer needs more info) » Needs work

There 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

pillarsdotnet’s picture

Status: Needs work » Postponed (maintainer needs more info)
Issue tags: -php5.3 deprecated, -php5.3

@anarcat: you didn't say whether the patch in #15 works for you. I, for one, would be interested in knowing.

anarcat’s picture

Status: Postponed (maintainer needs more info) » Needs review
FileSize
782 bytes

Yes, 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.

Status: Needs review » Needs work

The last submitted patch, no_ereg.patch, failed testing.

pillarsdotnet’s picture

Version: 6.19 » 6.x-dev
Component: install system » base system
Status: Needs work » Needs review
FileSize
4.41 KB

Patch suppresses warnings on all ereg functions still in 6.x core.

anarcat’s picture

Status: Needs review » Needs work

There seem to be unrelated whitespace changes in the patch.

pillarsdotnet’s picture

Status: Needs work » Needs review
FileSize
4.07 KB

Yeah; sorry about that. The emacs drupal.el auto-corrects certain whitespace errors without telling me about it.

Re-rolled without the gratuitous corrections.

pillarsdotnet’s picture

Grr... missed one.

kingandy’s picture

Line numbers are different (possibly because I'm on 6.20, possibly because I'm using Pressflow?), but those changes worked for me.

pillarsdotnet’s picture

Status: Needs review » Fixed

There are no ereg calls in a recent git checkout of Drupal 6.x. In fact, the last one was removed in September of 2008:

commit 89b4c55989eb0a1d2263de3a4a78c179f458cfee
Author: Angie Byron <webchick@24967.no-reply.drupal.org>
Date:   Sat Sep 20 03:49:24 2008 +0000

    #64967 follow-up by drewish: Replace ereg with preg in file_scan_directory().
kingandy’s picture

Status: Fixed » Needs review

@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.

pillarsdotnet’s picture

Sorry; something must have been wrong with my local clone. Had to delete and re-checkout the 6.x branch to fix.

Patch.

anarcat’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
2 KB

for 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.

j0nathan’s picture

Subscribing.

TravisCarden’s picture

Title: Cannot install a fresh 6.19 due to PHP deprecated messages » PHP warning: Function ereg() is deprecated in includes/file.inc
TravisCarden’s picture

Status: Reviewed & tested by the community » Needs work

Since 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.

pillarsdotnet’s picture

@#31 by TravisCarden on October 7, 2011 at 8:46pm:

Since ereg() has been deprecated ... replace it ... Surely that wouldn't be considered an API change, since it wouldn't affect the names or expected input or output of any functions.

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.

kingandy’s picture

For 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.

anarcat’s picture

Status: Needs work » Reviewed & tested by the community

No, 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...

TravisCarden’s picture

Satisfies me! Thank you.

pillarsdotnet’s picture

Somebody should write an issue summary. Also tagging "Novice" in a fit of wild optimism.

ShutterFreak’s picture

Well, 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.

anarcat’s picture

Issue summary: View changes

Updated issue summary.

anarcat’s picture

@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.

kattekrab’s picture

... ok I will have a go at writing up an issue summary!

kattekrab’s picture

oops - now I'm reading the issue summary... so someone's already done that it seems.

Dane Powell’s picture

I can also confirm the patch works, let's commit it...

kattekrab’s picture

tumbleweeds

theohawse’s picture

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

Committed 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.

theohawse’s picture

http://drupalcode.org/project/drupal.git/commit/ae71f5d

Heck yeah open source in action! Can't believe this made it to core :)

kattekrab’s picture

Yay! Thanks for doing the commit @Gábor Hojtsy

Status: Fixed » Closed (fixed)
Issue tags: -ereg deprecation

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.