Replace ereg with preg
meba - May 22, 2006 - 20:47
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | file system |
| Category: | task |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | closed |
Description
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?)
| Attachment | Size |
|---|---|
| file.inc_ereg_patch | 598 bytes |
#1
Well, i have a typo in the patch, attaching a new one
#2
Awesome idea. But the code needs work. Please fix the caller places to pass pregs.
image.inc:10: $toolkits = file_scan_directory('includes', 'image\..*\.inc$');system.module:591: foreach (file_scan_directory(dirname($theme->filename), 'style.css$') as $style) {system.module:689: * The key to be passed to file_scan_directory().system.module:707: $files = array_merge($files, file_scan_directory($dir, $mask, array('.', '..', 'CVS'), 0, TRUE, $key, $min_depth));system.module:547: $themes = system_listing('\.theme$', 'themes');system.module:550: $engines = system_listing('\.engine$', 'themes/engines');system.module:696:function system_listing($mask, $directory, $key = 'name', $min_depth = 1) {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 #.#3
I wanted to say < and > but an obvious typo stopped this.
#4
ok, will do next week, now i am working on my exams at university .)
#5
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
#6
<?php
// Code style. Should be
'^'. $module .'\...'
// not
'^' . $module . '\...'
?>
#7
#8
Not related to your patch specifically, but should we prefix the style line with a '^' to not match somestyle.css also?
#9
#10
I bet this does not apply to Drupal 6 at all. But a nice undertaking to start with :)
#11
I mean the patch does not apply, not the idea :) So the patch probably needs to be rerolled.
#12
#13
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.
#14
http://drupal.org/node/286893 was duplicate, but has a new patch. http://drupal.org/files/issues/drupal-preg-286893-4.patch
#15
It's easier to follow an issue when the patch is attached directly.
#16
Subscribing
#17
Still apply.
#18
If we do this then this additional issue should be addressed: #270824: Require preg_* functions for regular expressions.
#19
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!
Robin
#20
Committed to CVS HEAD. Thanks.
#21
did we intentionally skip the ereg() in file_scan_directory()?
#22
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.
#23
here's a first wack at this. i ran through the installer and enabled all the modules with no problems. running the tests now.
#24
finished the tests and found one regex i'd missed in simpletest.
#25
since it's only affecting the file_scan_dir now i'll bump the component.
#26
Hm. Could we not keep the regexes in the calling function the same, and surround the pattern with "/ ... /" in file_scan_directory()?
#27
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.
#28
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: API clean-up: 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.
#29
just submitted: http://drupal.org/node/224333#preg_match
#30
Automatically closed -- issue fixed for two weeks with no activity.