As a first small result of my effort to replace as much as i can handle occurences of ereg* functions by preg_match in Drupal ( http://groups.drupal.org/node/544 ), this is the first patch i am creating. Please let me know if i am doing things well as i am new to Drupal development :)

This patch removes ereg() in includes/file.inc at:

elseif ($depth >= $min_depth && ereg($mask, $file)) {

At my point of view, everything is working with preg_match().

Attaching patch (is this correct? should i put it elsewhere?)

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

meba’s picture

FileSize
600 bytes

Well, i have a typo in the patch, attaching a new one

chx’s picture

Status: Needs review » Needs work

Awesome idea. But the code needs work. Please fix the caller places to pass pregs.

  1. image.inc:10: $toolkits = file_scan_directory('includes', 'image\..*\.inc$');
  2. system.module:591: foreach (file_scan_directory(dirname($theme->filename), 'style.css$') as $style) {
  3. system.module:689: * The key to be passed to file_scan_directory().
  4. system.module:707: $files = array_merge($files, file_scan_directory($dir, $mask, array('.', '..', 'CVS'), 0, TRUE, $key, $min_depth));
  5. system.module:547: $themes = system_listing('\.theme$', 'themes');
  6. system.module:550: $engines = system_listing('\.engine$', 'themes/engines');
  7. system.module:696:function system_listing($mask, $directory, $key = 'name', $min_depth = 1) {
  8. system.module:893: $files = system_listing('\.module$', 'modules', 'name', 0);

In the future someone might change them and then it'll be crucial for them to be full pregs. Also, I really do not advice using a slash for a delimiter, because we are dealing with files, paths and such. Someone might write [^/] so I'd recommend you ^lt; > as delimiters. Or #.

chx’s picture

I wanted to say < and > but an obvious typo stopped this.

meba’s picture

ok, will do next week, now i am working on my exams at university .)

meba’s picture

Version: x.y.z » 5.x-dev
Component: file system » other
Category: bug » task
Status: Needs work » Needs review
FileSize
2.04 KB

Rerolling against 5.x. Searched for all eregs using file_scan_directory and should be fixed.

includes/common.inc:  file_scan_directory(file_create_path('css'), '.*', array('.', '..', 'CVS'), 'file_delete', TRUE);
includes/common.inc:    $files = array_merge($files, file_scan_directory($dir, $mask, array('.', '..', 'CVS'), 0, TRUE, $key, $min_depth));
includes/file.inc:function file_scan_directory($dir, $mask, $nomask = array('.', '..', 'CVS'), $callback = 0, $recurse = TRUE, $key = 'filename', $min_depth = 0, $depth = 0) {
includes/file.inc:          $files = array_merge($files, file_scan_directory("$dir/$file", $mask, $nomask, $callback, $recurse, $key, $min_depth, $depth + 1));
includes/image.inc:  $toolkits = file_scan_directory('includes', 'image\..*\.inc$');
includes/install.inc:    $installs = array_merge($installs, file_scan_directory('./modules', '^' . $module . '\.install$', array('.', '..', 'CVS'), 0, TRUE, 'name', 0));
install.php:  $profiles = file_scan_directory('./profiles', '\.profile$', array('.', '..', 'CVS'), 0, TRUE, 'name', 0);
install.php:  $locales = file_scan_directory('./profiles/' . $profilename, '\.po$', array('.', '..', 'CVS'), 0, FALSE);
modules/system/system.module:    foreach (file_scan_directory(dirname($theme->filename), 'style\.css$') as $style) {

Test please

RobRoy’s picture

Status: Needs review » Needs work
// Code style. Should be
'^'. $module .'\...'

// not

'^' . $module . '\...'
meba’s picture

Status: Needs work » Needs review
FileSize
2.04 KB
RobRoy’s picture

Not related to your patch specifically, but should we prefix the style line with a '^' to not match somestyle.css also?

drumm’s picture

Version: 5.x-dev » 6.x-dev
Gábor Hojtsy’s picture

I bet this does not apply to Drupal 6 at all. But a nice undertaking to start with :)

Gábor Hojtsy’s picture

I mean the patch does not apply, not the idea :) So the patch probably needs to be rerolled.

catch’s picture

Status: Needs review » Needs work
Pancho’s picture

Title: Replacing ereg with preg_match » Replacing ereg by preg_match
Version: 6.x-dev » 7.x-dev
Priority: Normal » Critical

Good start, indeed! We need to get away from ereg soon, as POSIX style regex are to be removed from PHP6.
However I think we should tackle this once and for all as a big task, therefore D7.
As this will break Drupal in PHP6, it is definitely a critical task.

catch’s picture

Title: Replacing ereg by preg_match » Replace ereg with preg
Assigned: meba » Unassigned
Status: Needs work » Needs review
cburschka’s picture

FileSize
4.64 KB

It's easier to follow an issue when the patch is attached directly.

Anonymous’s picture

Subscribing

lilou’s picture

Still apply.

Susurrus’s picture

If we do this then this additional issue should be addressed: #270824: Require preg_* functions for regular expressions.

Robin Monks’s picture

Status: Needs review » Reviewed & tested by the community

Please note, this issue will be affected by this as well: http://drupal.org/node/74645

+1 for replacing ereg with preg!

Patch still applies, and appears to work perfectly. RTBC!

Blog API functionality: 47 passes, 0 fails, 0 exceptions
Filter administration functionality: 95 passes, 0 fails, 0 exceptions
Core filters: 20 passes, 0 fails, 0 exceptions
Test single fields: 112 passes, 0 fails, 0 exceptions
Test select field: 33 passes, 0 fails, 0 exceptions
Test date field: 35 passes, 0 fails, 0 exceptions
Test field weights: 47 passes, 0 fails, 0 exceptions

Robin

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

drewish’s picture

did we intentionally skip the ereg() in file_scan_directory()?

Dave Reid’s picture

Status: Fixed » Active

Not sure if it was intentional, but it's definitely the last instance left in HEAD. I'll see if I can patch that to preg_match today.

drewish’s picture

Status: Active » Needs review
FileSize
14.56 KB

here's a first wack at this. i ran through the installer and enabled all the modules with no problems. running the tests now.

drewish’s picture

FileSize
12.07 KB

finished the tests and found one regex i'd missed in simpletest.

drewish’s picture

Component: other » file system

since it's only affecting the file_scan_dir now i'll bump the component.

webchick’s picture

Hm. Could we not keep the regexes in the calling function the same, and surround the pattern with "/ ... /" in file_scan_directory()?

drewish’s picture

webchick, we could do that but we'd loose the ability to use case insensitive regular expressions. And this is a more honest way of doing it that won't lead to subtle bugs where ereg expressions fail with preg.

webchick’s picture

Status: Needs review » Needs work

Well, that's true. And also, people could also use a different delimiter like # if they wanted to not have to escape /s or whatnot.

Reading this code reminds me that #255551: DX: Array-itize file_scan_directory()'s parameters and #74645: modify file_scan_directory to include a regex for the nomask. would both be nice improvements to file_scan_directory(). Those 120+ character function calls are nasty. ;)

Re-ran the file tests and everything looks good, so committed this. Thanks!

The change to file_scan_directory()'s parameters needs to be noted in the upgrade docs, so marking this code needs work until then.

drewish’s picture

Status: Needs work » Fixed
Anonymous’s picture

Status: Fixed » Closed (fixed)

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

donquixote’s picture

What can we do for 6.x ?
I see this issue being closed since October 2008, but in 6.x we still have ereg() instead of preg_match(). With all the nasty errors in PHP 5.3.

Should we make a new issue for 6.x, or switch this one from 7.x to 6.x ?
I would like to point out that the issue was reported for x.y.z (?), then switched to 5.x, then to 6.x. Now it's fixed in D7, but then was forgotten with no backport for almost one year! Is there something wrong with the issue workflow?

EDIT:
Would "@ereg" instead of "ereg" be ok as a temporary fix?

Dave Reid’s picture

Using preg() instead of ereg() in Drupal 6 would break existing APIs, so it will never happen. Your best bet is to set your PHP settings to not show E_DEPRECATED errors. See http://us2.php.net/manual/en/function.error-reporting.php

sinasquax’s picture

Version: 6.15 » 7.x-dev
Status: Needs review » Closed (fixed)

We can keep the compatibility and adds this feature by adding an extra parameter to the function :


function file_scan_directory($dir, $mask, $nomask = array('.', '..', 'CVS'), $callback = 0, $recurse = TRUE, $key = 'filename', $min_depth = 0, $depth = 0, $usePreg = FALSE) {

// Find the right function
$regFunc = ($usePreg) ? 'preg_match' : 'ereg';

// And replace :
elseif ($depth >= $min_depth && ereg($mask, $file)) {

// By :
elseif ($depth >= $min_depth && $regFunc($mask, $file)) {

With this, the old functions which use file_scan_directory will not break and the new functions which need preg can set the last parameter to TRUE

sinasquax’s picture

Version: 7.x-dev » 6.15
Status: Closed (fixed) » Needs review

Version: 7.x-dev » 6.15
Status: Closed (fixed) » Needs work

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

Druid’s picture

Using preg() instead of ereg() in Drupal 6 would break existing APIs, so it will never happen.

It has to happen sooner or later. The "ereg" family of functions (as well as many other "deprecated" functions) are going away in PHP 6.0. Gone. Period. I suppose you could suppress the warning messages until then, and add emulation wrappers for PHP 6+ users at the time they upgrade to 6, but why not bite the bullet and do it right, by changing all the "ereg" family calls?

Damien Tournoud’s picture

Status: Needs work » Closed (fixed)

ereg() is still marked as deprecated, but it's not going away.

Druid’s picture

Has PHP changed its plans? ereg is still marked as "deprecated", which means they plan(ned) to remove it at some point. Last I heard, that was PHP 6.0 (which isn't that far off).

Damien Tournoud’s picture

PHP 6 doesn't exist (and will probably never exists), and ereg is not scheduled to be removed anymore because the full Unicode support is not on the table anymore.

Anyway, Drupal 6 will probably be end-of-life before PHP vNext makes its way to major LAMP distributions.

Druid’s picture

Ah, I had not heard that PHP 6 development has been suspended. Is there any official word on what they plan to do in future releases? Are "ereg" family functions, among other things, still scheduled to be removed at some point in the near future? I would assume, until otherwise officially informed, that the PHP 6 effort is not dead, but merely in stand-down for some period while they regroup and come up with a new roadmap.

In the meantime, barring any official PHP word to the contrary, it would be safe to assume that "ereg" is going away and that "preg_*" should be substituted for it. If this has not been done before word comes down from PHP that "ereg" is here to stay, then no big deal. If it's done, and "ereg" is declared to stay, no great loss. However, if "ereg" is removed some time soon, it could make a mess if Drupal isn't ready for it! I'm not sure I'm ready to play the odds and risk that "ereg" is permanently staying in PHP, absent official word that it is.

Damien Tournoud’s picture

Druid: to be very clear:

  • ereg is not scheduled for removal anymore (read the thread),
  • deprecated means "don't write new code with this", and Drupal 6 is not new code
  • Drupal 6 is API frozen, so those ereg calls will not be removed from this core version,
  • If ever ereg is removed from a future PHP version, Drupal 6 will simply not support this version,
  • Drupal 7 doesn't use ereg anymore.