Posted by meba on May 22, 2006 at 8:47pm
| Project: | Drupal core |
| Version: | 6.15 |
| Component: | file system |
| Category: | task |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
Issue Summary
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 | Status | Test result | Operations |
|---|---|---|---|---|
| file.inc_ereg_patch | 598 bytes | Ignored: Check issue status. | None | None |
Comments
#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: 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.
#29
just submitted: http://drupal.org/node/224333#preg_match
#30
Automatically closed -- issue fixed for two weeks with no activity.
#31
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?
#32
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
#33
We can keep the compatibility and adds this feature by adding an extra parameter to the function :
<?php
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
#34
#35
The last submitted patch, preg_64967.patch, failed testing.
#36
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?
#37
ereg() is still marked as deprecated, but it's not going away.
#38
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).
#39
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.
#40
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.
#41
Druid: to be very clear: