I am not sure if there are any plans regarding php 5.3 at this time.

I am currently testing php5.3.0alpha3 and found several issues with current HEAD. Most of them are because PHP 5.3 seems to create a warning message when a function with by-reference parameters is called by value. For example, this does happen when a hook defines his parameters as by-reference but is called with module_invoke(_all).

These errors are pretty easy to fix, however, there are some others where I'm not sure.

- PHP 5.3 changed the wording of the warning message "The second argument should be either an array or an object" to "array_key_exists() expects parameter 2 to be array, null given ". Because this warning mesage is used by simpletest.test to ensure that php warnings are correctly reportet, the test does now fail. I don't know how this should be resolved so that it does work with php 5.3 and older versions too.

- Some of the upload user image tests are broken. I haven't looked at those in detail yet. This is maybe another issue or a misconfiguration/bug of gd because there are no php warnings reportet, in contrast to all other fails.

- Some aggregator tests are broken, because of a by reference warning, but I'm unable to figure out where the function is called. For some reason, $form_state is given by value instead of by reference and only for one single form of the whole test suite

I'm attaching a first patch which does fix most of the by reference warnings I've found, a (maybe incomplete) list:
- multiple hook_user_cancel
- multiple hook_user_categories
- multiple hook_user_logout
- theme_upload_form_current
- theme_status_report
- multiple file_validate_* functions

No Idea which category this belongs, because multiple modules are affected.

Files: 
CommentFileSizeAuthor
#233 php53_remove_deprecated.patch684 bytesBerdir
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch php53_remove_deprecated.patch.
[ View ]
#176 drupal.php53_compat_03-D5.patch5.38 KBandrewfn
#175 drupal.php53_compat_02-D5.patch5.62 KBandrewfn
#173 drupal.php53_compat_01-D5.patch8.08 KBandrewfn
#157 drupal.php53_compat_10-D6.patch6.83 KBBerdir
#84 drupal.php53_compat_9-D6.patch6.43 KBBerdir
#76 drupal.php53_compat_8-D6.patch6.49 KBBerdir
#49 drupal.php53_compat_7-D6.patch6.49 KBBerdir
#36 drupal.php53_compat_6-D6.patch5.91 KBBerdir
#34 drupal.php53_compat_5-D6.patch11.74 KBBerdir
#30 drupal.php53_compat_4-D6.patch7.28 KBBerdir
#29 drupal_user_edit_page.jpg116.71 KBpenguins
#27 drupal.php53_compat_3-D6.patch12.04 KBBerdir
#25 drupal.php53_compat_2-D6.patch12.01 KBBerdir
#22 drupal.php53_compat_1-D6.patch18.44 KBBerdir
#15 php53_warnings_4.patch11.62 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch php53_warnings_4.patch.
[ View ]
#11 php53_warnings_3.patch11.76 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch php53_warnings_3.patch.
[ View ]
#6 php53_warnings_2.patch10.72 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch php53_warnings_2.patch.
[ View ]
Simpletest test fail15.07 KBBerdir
User upload test fails22.58 KBBerdir
Aggregator test fails126.3 KBBerdir
php53_warnings.patch9.29 KBBerdir
Test request sent.
Previous result: Passed: 8991 passes, 0 fails, 0 exceptions
[ View ]

Comments

I do not readily understand "warning message when a function with by-reference parameters is called by value". You have function x(&$foo) and then you call it with x($bar) and that $foo in the scope of x is a reference is bar is determined by the fact that x(&$foo) is defined as a reference. Now, if you say that call_user_func_array('x', array($bar)) throws a warning then PHP 5.3 is utterly stupid and I would consider it a bug and file a bug on php.net.

Oh and thanks a ton for working on this!

Thanks for working on the issue. The PHP team confirms this and considers it a good thing 'cos earlier it failed silently so yeah this is a valid bug report. sigh.

Issue tags:+PHP, +PHP 5.3

Yay references! Following to watch progress.

Priority:Normal» Critical

Bumping to critical - while 5.3 might not get much use before Drupal 7 is released, it's going to be out for some time before Drupal 9 is released and we drop support for D7. Would be better to have this as a release blocker to avoid dealing with it in a point release at some point.

Priority:Critical» Normal
StatusFileSize
new10.72 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch php53_warnings_2.patch.
[ View ]

Some Updates:

- For the record, bug report about the warning message, was closed as bogus.
http://bugs.php.net/bug.php?id=43484

- aggregation fails were actually a drupal_execute "bug", because drupal_execute didn't consider $form_state as by-reference. It seems that the aggregator tests are the only place in the test suite where drupal_execute is used.

- upload profile image failed because I compiled gd without jpeg support

- another by-reference fix in comment_user_cancel

Still open:

- PHP 5.3 changed the wording of the warning message "The second argument should be either an array or an object" to "array_key_exists() expects parameter 2 to be array, null given ". Because this warning mesage is used by simpletest.test to ensure that php warnings are correctly reportet, the test does now fail. I don't know how this should be resolved so that it does work with php 5.3 and older versions too.

I tried to find a function with an unchanged warning but had no luck so far.

Priority:Normal» Critical

Re-setting priority to critical.

Marked #253023: status report, etc. broken under php 5.3 as a duplicate of this issue.

Status:Needs work» Needs review

Ok, instead of tricking PHP to issue a warning message, I'm currently using trigger_error. This is not exactly the same as what has been tested before, but it has the advance that it does test (only) the parsing/testing of error messages and not the wording of them.

With this change, all tests pass now, so I'm setting this to CNR.

The tests finished in 13 min 37 sec.
8924 passes, 0 fails, and 0 exceptions

1337... this *has* to be good :)

Status:Needs review» Active

Would be better with a patch :)

Status:Active» Needs review
StatusFileSize
new11.76 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch php53_warnings_3.patch.
[ View ]

Forgot the patch...

This patch looks good to me. Will commit after another review.

Two nitpicks:

<?php
    
// Generates a warning inside a PHP function.
-    array_key_exists(NULL, NULL);
+   
trigger_error('Simpletest test warning message', E_USER_WARNING);
   }
?>

This is not exactly the same. The idea here was to generate an error *inside* a PHP internal function. This is in fact a regression test: our backtracing code wasn't able to deal with this situation at some point. Could we find an error message that haven't changed between the two versions?

<?php
+
// Use by-reference parameter of $form_state
$args[1] = &$form_state;
+
?>

This sentence should end with a dot.

Status:Needs review» Needs work

> Could we find an error message that haven't changed between the two versions?
I tried :) The issue is that every single simple "wrong input"-warning seems to have been changed. Tried string, array, file.

For example, the changes of array_key_exists:

$ php -r'array_key_exists(NULL, NULL);'
Warning: array_key_exists(): The second argument should be either an array or an object in Command line code on line 1
$ php53 -r'array_key_exists(NULL, NULL);'
Warning: array_key_exists() expects parameter 2 to be array, null given in Command line code on line 1

What we could do is not test the whole string but only the function name "array_key_exists". This should be enough to be sure that the warning is there and the location is correct.

Status:Needs work» Needs review
StatusFileSize
new11.62 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch php53_warnings_4.patch.
[ View ]

Ok, new try, changes:

- Fixed comment in drupal_execute
- Use only 'array_key_exists' to test the warning message as described above and added a comment to explain why, similiar to common.test

Status:Needs review» Fixed

Works! Committed to CVS HEAD.

Version:7.x-dev» 6.x-dev
Status:Fixed» Needs review

Making Drupal 6 php 5.3 compatible is more complicated, mostly because of the following reason:

hook_user (maybe others too) is sometimes called with module_invoke (=by value) and sometimes with user_module_invoke (= by reference). This means that I can't simply remove the & from the hook definitions, as it is possible in Drupal 7 thanks to the removal of the $op switch.

These are the module_invoke(_all) calls in the user module:

modules/user/user.module:  module_invoke_all('user', 'delete', $edit, $account);
modules/user/user.module:    if ($data = module_invoke($module, 'user', 'categories', NULL, $account, '')) {
modules/user/user.module:    if ($data = module_invoke($module, 'user', $hook, $edit, $account, $category)) {
modules/user/user.pages.inc:  module_invoke_all('user', 'logout', NULL, $user);

( The third is in the function _user_forms and $hook defaults to form)

Discussed with chx in irc, there are 2 options:

1) Change atleast (there may be other hooks with the same issue, I've not yet tested everything) all calls of hook_user to user_module_invoke. I haven't tested this yet but it should work. But it is a fairly big change and does actually "break" the api. In the case of hook_user, $account will always be by-reference in contrast to the current version. This is what you would expect anyway but it could still change/break something if a contrib module changes $account for some reason.

2) This is simple, don't support 5.3 with drupal 6. While the actual error is "only" a warning, it does break things because all by reference parameters are set to NULL.

----------------

Another issue is E_DEPRECATED. However, there are simply two ways to deal with it and it's not about doing it or not. E_DEPRECATED has been added to PHP 5.3 and in contrast to E_STRICT, it is included in E_ALL and is enabled by default (atleast in the current alpha3 version). Deprecated is, beneath others especially ereg. See http://wiki.php.net/doc/scratchpad/upgrade/53 for a full list.

ereg functions are still used in Drupal 6 in multiple places:

grep -R ereg includes/ modules/ | grep -v CVS
includes/unicode.inc:  if (!$bom && ereg('^<\?xml[^>]+encoding="([^"]+)"', $data, $match)) {
includes/unicode.inc:      $data = ereg_replace('^(<\?xml[^>]+encoding)="([^"]+)"', '\\1="utf-8"', $out);
includes/file.inc:    $regex = '/\.('. ereg_replace(' +', '|', preg_quote($extensions)) .')$/i';
includes/file.inc:        elseif ($depth >= $min_depth && ereg($mask, $file)) {
modules/user/user.module:  if (ereg("[^\x80-\xF7 [:alnum:]@_.-]", $name)) return t('The username contains an illegal character.');
modules/user/user.module:  if (strpos($name, '@') !== FALSE && !eregi('@([0-9a-z](-?[0-9a-z])*.)+[a-z]{2}([zmuvtg]|fo|me)?$', $name)) return t('The username is not a valid authentication ID.');
modules/blogapi/blogapi.module:  if (eregi('<title>([^<]*)</title>', $contents, $title)) {
modules/blogapi/blogapi.module:    $contents = ereg_replace('<title>[^<]*</title>', '', $contents);

1. Easy way: exclude E_DEPRECATED from being being displayed/logged. I'm quite sure this is the way to go (ereg has been replaced in drupal 7 already, so...) but I don't wan't to make that decision :)

2. "Fix" these "errors" be replacing the ereg functions with preg_*

---

Note: Other issues might come up, these are the two that I found so far.

Status:Needs review» Needs work

Forgot to set the right status

It seems quite possible that we'll require PHP 5.3 in Drupal 8. IMO it'd be a pretty bad situation to not support it for Drupal 6, but require it for Drupal 8 - it'd prevent an upgrade from Drupal 6 to Drupal 8 on the same server - and many sites upgrade every other Drupal version. Passing $user by reference in a few places doesn't seem like too drastic of an API change to put this off, and the ereg to preg fixes could equally backported quite safely.

PHP 5.3 RC1 was released today, and there are some test failures in D7 in our exception handling and e-mail validation tests. It however seems that both of them are bugs in PHP:

See:
filter_var & e-mail validation: http://bugs.php.net/bug.php?id=47772
autoload inside error handler: http://bugs.php.net/bug.php?id=47714

Feel free to vote and/or contribute on these issues, if you can :)

I'm trying to create a patch for D6 now.

Status:Needs work» Needs review
StatusFileSize
new18.44 KB

Ok, first attempt of a D6 patch. I tried to backport from D7 the ereg* related stuff and some things of my patch which was commited against D7. This, especially the ereg* changes, needs a very thorough review, as there are almost no tests to validate that it works correctly.

Changes:
- Fix drupal_execute, backport of my patch
- fix theme calls which use by reference params, backport of my patch
- Change module_invoke_all() calls to user_module_invoke()
- Change module_invoke calls to $function (only possible way, because we need by reference and return value, no existing function does offer that)
- Replace all ereg* calls with preg_* (in one case, it seems that ereg_replace() can be replaced by just str_replace), backported from D7
- Fix all calls to file_scan_directoy() and drupal_system_listing() to use preg-style regex, backported from D7
- Replaced user_validate_name with the D7 version. The whole function in D6 is very hard to understand and doesn't follow the coding style. However, one check seems to miss in the D7 version, and I don't know why:
-  if (strpos($name, '@') !== FALSE && !eregi('@([0-9a-z](-?[0-9a-z])*.)+[a-z]{2}([zmuvtg]|fo|me)?$', $name)) return t('The username is not a valid authentication ID.');
If that is still needed, someone needs to convert that to preg_match, that's too much regex for me :)

Typo in a few places: 'refernce'.

I do think some sort of validation on authentication id is needed in d6. that regex is too much for me too.

I just noticed that we have a problem here.

drupal_system_listing() and file_scan_directory() expose their ereg regex, this means that it will break every module which uses this function, for example SimpleTest.

I think we can't do that with D6, so we probably have to go with option 1) as described in #18.

StatusFileSize
new12.01 KB

- Changed back to ereg() in file_scan_directory() and changed back all calls to it
- Fixed "refernce" typos
- If the authentication ID is really needed, someone else needs to write that, I can't do it on my own, I don't even know for what that is used :)

StatusFileSize
new12.04 KB

@26
Copy paste error, try the updated patch.

Someone up to the conversion of the regex in #22? If we really need it and all else fails, I can re-add the ereg() call.. we can't remove all of them anyway.

So when I tried to load the status report on a D6 website w/ PHP 5.3, I got a fatal error. Checking the logs I found a message (in addition to a lot of ereg warnings) similar to this:

Location http://www.example.com/admin/reports/status
Message Parameter 1 to theme_status_report() expected to be a reference, value given in /var/www/d6upgrade/includes/theme.inc on line 617

As of PHP 5.3, PHP will replace an argument mistakenly passed by value instead of reference with a NULL argument, whereas in PHP 5.2 or earlier it would just continue as if nothing happened. Presumably passing a NULL argument caused some sort of fatal error down the line.

Granted, this was on an experimental website where we were working on an upgrade from D5 to D6 (and it looks like we're on 6.9 on that website), but can anyone replicate this, or is it just something specific on my end?

Update: Never mind, it's fixed as of Drupal 6.10

StatusFileSize
new116.71 KB

Thank your for the patch, Berdir.

I've applied the patch. User edit page is now showing the account informations form, however, it doesn't show user current's username and email address. Please check out the attached picture.

I have successfully changed username and password, the patch also fix status report issue.

StatusFileSize
new7.28 KB

@28
This is a known issue and should be fixed with my patch.

@29
Another copy paste error it seems. Try the updated patch, it should be fixed now. Thanks for testing this, it seems I'm not able to think myself :) No idea why I didn't noticed the empty form.

patch need to be unified diff format

StatusFileSize
new11.74 KB

Oh, updated patch...

Status:Needs review» Needs work

why did you change the regex? It's not neccesary to do so for a 6.x fix, and woudl seem to risk changes in behavior.

for example:

-  if (!$bom && ereg('^<\?xml[^>]+encoding="([^"]+)"', $data, $match)) {
+  if (!$bom && preg_match('/^<\?xml[^>]+encoding="(.+?)"/', $data, $match)) {

and

-  if (eregi('<title>([^<]*)</title>', $contents, $title)) {
+  if (preg_match('/<title>(.*?)<\/title>/i', $contents, $title)) {
     $title = strip_tags($title[0]);
-    $contents = ereg_replace('<title>[^<]*</title>', '', $contents);
+    $contents = preg_replace('/<title>.*?<\/title>/i', '', $contents);

Also, typically if the regex contains '/' we use a different delimiter such as '@':

$contents = preg_replace('@<title>[^<]*</title>@i', '', $contents);

StatusFileSize
new5.91 KB

The plan was to remove ereg() because that is marked as deprecated on PHP 5.3. However, it is not possible to do that, because some ereg() calls expose their regex (see #24) so I reverted these calls and excluded E_DEPRECATED errors from being displayed.

I agree that it does not make sense to partially remove ereg() then, so I reverted all ereg related changes back.

The updated patch is now pretty simple and only does the following things

- If necessary, defines a E_DEPRECATED constant
- excludes all E_DEPRECATED errors from getting displayed
- removes & from theme functions (because of a change with by reference handling in PHP 5.3, already in D7)
- The fix for drupal_execute (because of the same change, already in D7)
- and some user hook invocation changes (because of the same change, solved slightly different in D7 because of the renamed hooks)

Also improved coding style of the comments a bit, this should look a lot better now..

Status:Needs work» Needs review

Setting to needs review again..

Status:Needs review» Needs work

looks better, but this comment needs some work:

// We can't use neither module_invoke nor user_module_invoke because we need the return value and by reference.

Parameter 1 to profile_load_profile() expected to be a reference, value given in /includes/module.inc on line 450

This is not addressed by this patch I believe.

@killes

Can you tell me how to reproduce that error and have you tested it with the patch? I created some profile fields, and everything seems to work fine.

yes, I had applied this patch. I currently cannot reproduce it since I've gone back to 5.2 for now.

I think all you need to do is to remove the & in the function definition of profile_load_profile.

Does Drupal 6.x support PHP5.3?

I have downloaded yesterday's snapshot of Drupal6-x-dev, found out that non of the patches have been added to it. So I'm wondering whether Drupal 6 is going to support PHP 5.3.

Hi,

Installing the patch I get the following:


patch -b -p0 < drupal.php53_compat_6-D6.patch
patching file includes/common.inc
Hunk #2 FAILED at 584.
1 out of 2 hunks FAILED -- saving rejects to file includes/common.inc.rej
patching file includes/form.inc
patching file modules/system/system.admin.inc
patching file modules/upload/upload.module
patching file modules/user/user.module
patching file modules/user/user.pages.inc

Here are the contents of common.inc.rej:

***************
*** 577,583 ****
      return;
    }
-   if ($errno & (E_ALL)) {
      $types = array(1 => 'error', 2 => 'warning', 4 => 'parse error', 8 => 'notice', 16 => 'core error', 32 => 'core warning', 64 => 'compile error', 128 => 'compile warning', 256 => 'user error', 512 => 'user warning', 1024 => 'user notice', 2048 => 'strict warning', 4096 => 'recoverable fatal error');
      // For database errors, we want the line number/file name of the place that
--- 584,590 ----
      return;
    }
+   if ($errno & (E_ALL ^ E_DEPRECATED)) {
      $types = array(1 => 'error', 2 => 'warning', 4 => 'parse error', 8 => 'notice', 16 => 'core error', 32 => 'core warning', 64 => 'compile error', 128 => 'compile warning', 256 => 'user error', 512 => 'user warning', 1024 => 'user notice', 2048 => 'strict warning', 4096 => 'recoverable fatal error');
      // For database errors, we want the line number/file name of the place that

Is the portion that's failing the part that addresses the ereg() problem?

The ereg() thing really makes drupal nearly unusable with errors being displayed. Is there going to be an actual fix for this at some point?

Thanks.

Try using drupal.php53_compat_5-D6.patch. You can set the errors to be written to a log file in the administrer -> Site Configuration -> Error reporting, that way the pages will not be cluttered with the error messages.

The patch is against the -dev version, because that does not contain the additional E_NOTICE check. However, you can manually overwrite the failing line.

The patch is not yet commited. killes reported a bug with profiles that I can't reproduce. If you try out the patch, please report any error messages or broken things that you can find.

Changing the error reporting helps. I'm a newbie - where are the error logs located in a default configuration? Is there a way to control where the logs are written to?

I still get this error applying the version 5 patch:

"
Hunk #2 FAILED at 584.
1 out of 2 hunks FAILED -- saving rejects to file includes/common.inc.rej
"

Searching for 584 in the patch file I find:

@@ -577,7 +584,7 @@ function drupal_error_handler($errno, $m

What would I need to do to fix this "hunk?"

I don't know anything about patch or how it works really, but it looks like this line is incomplete (no terminating ")" at the very least)?

This time the common.inc.rej contents are:

***************
*** 577,583 ****
      return;
    }
-   if ($errno & (E_ALL)) {
      $types = array(1 => 'error', 2 => 'warning', 4 => 'parse error', 8 => 'notice', 16 => 'core error', 32 => 'core warning', 64 => 'compile error', 128 => 'compile warning', 256 => 'user error', 512 => 'user warning', 1024 => 'user notice', 2048 => 'strict warning', 4096 => 'recoverable fatal error');
      // For database errors, we want the line number/file name of the place that
--- 584,590 ----
      return;
    }
+   if ($errno & (E_ALL ^ E_DEPRECATED)) {
      $types = array(1 => 'error', 2 => 'warning', 4 => 'parse error', 8 => 'notice', 16 => 'core error', 32 => 'core warning', 64 => 'compile error', 128 => 'compile warning', 256 => 'user error', 512 => 'user warning', 1024 => 'user notice', 2048 => 'strict warning', 4096 => 'recoverable fatal error');
      // For database errors, we want the line number/file name of the place that

Thanks again.

gr00vus,

which version of Drupal are you patching?

One way to apply the patches is to delete the lines with "-" sign in front (from the common.inc.rej) and replace it with the lines with "+" sign in front.

Berdir,

I'm testing using Windows Server 2008 and PHP 5.3 RC3-dev and I found the following errors (after applying drupal.php53_compat_5-D6.patch)

1. Unable to change Garland theme's colour scheme (Go to Administer -> site building -> themes -> click on "configure" on the right hand site and Garland theme -> change colour scheme to anything other than Blue Lagoon (default)).
Found the following error msg:

The selected file could not be copied, because no file by that name exists. Please check that you supplied the correct filename.

2. Unable to change website logo (Go to Administer -> site building -> themes -> click on "configure" on top of the page -> upload an image on Logo Image Settings).
Found the following error msgs:
warning: Parameter 1 to file_validate_is_image() expected to be a reference, value given in C:\inetpub\wwwroot\drupal6-patched\includes\file.inc on line 553.
warning: array_merge(): Argument #2 is not an array in C:\inetpub\wwwroot\drupal6-patched\includes\file.inc on line 553.
warning: array_merge(): Argument #1 is not an array in C:\inetpub\wwwroot\drupal6-patched\includes\file.inc on line 553.
The selected file could not be copied, because no file by that name exists. Please check that you supplied the correct filename.

3. Unable to change website shortcut icon (Go to Administer -> site building -> themes -> click on "configure" on top of the page -> upload an image on Shortcut Item Settings).
Fount the following error msg:
The selected file could not be copied, because no file by that name exists. Please check that you supplied the correct filename.

4. I am able to attach a file to a content, but it finishes with several warnings:
warning: Parameter 1 to file_validate_image_resolution() expected to be a reference, value given in C:\inetpub\wwwroot\drupal6-patched\includes\file.inc on line 553.
warning: array_merge(): Argument #2 is not an array in C:\inetpub\wwwroot\drupal6-patched\includes\file.inc on line 553.
warning: array_merge(): Argument #1 is not an array in C:\inetpub\wwwroot\drupal6-patched\includes\file.inc on line 553.
warning: array_merge(): Argument #1 is not an array in C:\inetpub\wwwroot\drupal6-patched\includes\file.inc on line 553.

Status:Needs work» Needs review
StatusFileSize
new6.49 KB

Thanks for your tests..

1. Unable to change Garland theme's colour scheme (Go to Administer -> site building -> themes -> click on "configure" on the right hand site and Garland theme -> change colour scheme to anything other than Blue Lagoon (default)).
Found the following error msg:

The selected file could not be copied, because no file by that name exists. Please check that you supplied the correct filename.

I can't reproduce this. Maybe something is wrong with your write permissions?

2. Unable to change website logo (Go to Administer -> site building -> themes -> click on "configure" on top of the page -> upload an image on Logo Image Settings).
Found the following error msgs:
warning: Parameter 1 to file_validate_is_image() expected to be a reference, value given in C:\inetpub\wwwroot\drupal6-patched\includes\file.inc on line 553.
warning: array_merge(): Argument #2 is not an array in C:\inetpub\wwwroot\drupal6-patched\includes\file.inc on line 553.
warning: array_merge(): Argument #1 is not an array in C:\inetpub\wwwroot\drupal6-patched\includes\file.inc on line 553.
The selected file could not be copied, because no file by that name exists. Please check that you supplied the correct filename.

Confirmed and fixed.

3. Unable to change website shortcut icon (Go to Administer -> site building -> themes -> click on "configure" on top of the page -> upload an image on Shortcut Item Settings).
Fount the following error msg:
The selected file could not be copied, because no file by that name exists. Please check that you supplied the correct filename.

Again, I can't reproduce this.

4. I am able to attach a file to a content, but it finishes with several warnings:
warning: Parameter 1 to file_validate_image_resolution() expected to be a reference, value given in C:\inetpub\wwwroot\drupal6-patched\includes\file.inc on line 553.
warning: array_merge(): Argument #2 is not an array in C:\inetpub\wwwroot\drupal6-patched\includes\file.inc on line 553.
warning: array_merge(): Argument #1 is not an array in C:\inetpub\wwwroot\drupal6-patched\includes\file.inc on line 553.
warning: array_merge(): Argument #1 is not an array in C:\inetpub\wwwroot\drupal6-patched\includes\file.inc on line 553.

This was the same bug as above.

@killes
I found your bug I think.

sites/all/modules/realname/realname.module:    module_invoke($module, 'load_profile', $account, $type);

Or maybe another module... This is not a bug in core. That should be something like
<?php
$function
= $module . '_load_profile';
$function($account, $type);
?>

Part of this patch has to be applied manually to a drupal 6.12 installation:
In common.inc, line 577, drupal 6.x CVS has
if ($errno & (E_ALL))
where drupal 6.12 has
if ($errno & (E_ALL ^ E_NOTICE))

This change appeared in rev 1.756.2.53, without being mentioned in the logs.

That is the difference between a stable and a -dev release, dev releases display E_NOTICE messages.

@penguins

I'm using version 6.12. I realize the patch is targeted at the development version, which probably explains the patch errors - should I switch over to using the development version instead? I'm not making a production site (yet), it's mainly personal use right now, though it may end up with a wider audience in the near future. I figure I'd start with the stable version.

Thanks.

Berdir,

Thank you for the patch, it stopped the warning. You are right about the file permission, the PHP upload_tmp_dir dir is not readable by user. I can now change theme's colour and change the website logo.

I found another warning message when trying to change content's status. To replicate, go to Administer -> Content Management -> tick several titles and change the update options (from publish to unpublish or the other way).

Warnings found:

notice: Undefined property: stdClass::$new in C:\inetpub\wwwroot\drupal6-patched\modules\upload\upload.module on line 410.

gr00vus,

If you are going to turn the site into a production site, then it is better for you to use the stable version of PHP and Drupal (in this case PHP 5.2.9 and Drupal 6.12).

Just as a note, PHP 5.3 will become stable soon, probably in this week: http://news.php.net/php.internals/44397

So, please report back if it the patch works or if you have other issues.

@53
that is a E_NOTICE, that is now displayed because you use the -dev version (or changed the relevant check in the error callback function). This *is* a bug, but not related to this issue nor PHP 5.3. This notice won't be displayed with a stable version.

Edit: Also, to get this commited, we need confirmation that it still works on PHP4 with my patch. It *should*, but I haven't tested it. If anyone does still have a PHP4 install, please test it with this patch applied. Especially theme settings, profile stuff, and file/image uploads with automatic size conversion.

I think that a contrib project that adds 5.3 compatibility is better. No big ISP or distro will feature PHP 5.3 soon (heck, we needed to fight a war for PHP 5.2 for D7 and RHEL still do not even have that!) so if you run 5.3 you are running your own PHP. Then run your own Drupal too.

@chx - are you suggesting that the currently shipping drupal should not run on the currently shipping PHP? i can't imagine thats a good idea. i agree that this is critical. people setup their servers all the time and many of them run php 5.3.

On the same page, how exactly would a contrib module solve this? I can't think of a good way to do what this patch does via a contributed project without hacking core. Plus, looking at the patch, it has a very low risk of breaking anything, so our existing user base who's not on PHP 5.3 should not be affected.

Also, many devs use both a live and development server, with the dev server running the edgier version of PHP. And it would be hard to do development there if issues like these break Drupal.

people setup their servers all the time and many of them run php 5.3. <= wow. who ??? I would not touch PHP 5.3 with a ten feet barge pole for another 4-5 minor releases.

The contrib project should contain this patch. As said, if you run your own server, you can run your own Drupal.

I think the points chx makes are valid for Drupal 5. By the time 7 is released and 5 is no longer supported, no major ISP or distro will have PHP 5.3. However, since 6 will be supported until Drupal 8 comes out (quite a long time), it probably has to be patched for PHP 5.3 at some point in time.

Status:Needs review» Reviewed & tested by the community

Berdir says the latest patch does away with the hook_user API break. Aye. So let's go then.

PHP 5.3 deprecates the ereg library, which appears to be in use in a disturbing number of places in core:

$ grep -n 'ereg' -r ./
./includes/file.inc:626:    $regex = '/\.('. ereg_replace(' +', '|', preg_quote($extensions)) .')$/i';
./includes/file.inc:895:        elseif ($depth >= $min_depth && ereg($mask, $file)) {
./includes/unicode.inc:138:  if (!$bom && ereg('^<\?xml[^>]+encoding="([^"]+)"', $data, $match)) {
./includes/unicode.inc:148:      $data = ereg_replace('^(<\?xml[^>]+encoding)="([^"]+)"', '\\1="utf-8"', $out);
./modules/blogapi/blogapi.module:692:  if (eregi('<title>([^<]*)</title>', $contents, $title)) {
./modules/blogapi/blogapi.module:694:    $contents = ereg_replace('<title>[^<]*</title>', '', $contents);
./modules/user/user.module:385:  if (ereg("[^\x80-\xF7 [:alnum:]@_.-]", $name)) return t('The username contains an illegal character.');
./modules/user/user.module:398:  if (strpos($name, '@') !== FALSE && !eregi('@([0-9a-z](-?[0-9a-z])*.)+[a-z]{2}([zmuvtg]|fo|me)?$', $name)) return t('The username is not a valid authentication ID.');
./sites/all/modules/logintoboggan/logintoboggan.module:855:  if (ereg("[^\x80-\xF7[:graph:] ]", $pass)) return t('The password contains an illegal character.');
./sites/all/modules/pathauto/pathauto.inc:183:  if (function_exists('mb_eregi_replace')) {
./sites/all/modules/pathauto/pathauto.inc:184:    $output = mb_eregi_replace($ignore_re, '', $output);

I'm crushing, so I'll just exclude E_DEPRECATED from error_reporting for now, but it would be good to see these swapped out with their preg equivalents.

EDIT: Apparently you can't exclude E_DEPRECATED? Rrgh, I wish this friggin' client wouldn't insist on using their own in-house Windows server.

Changing from ereg to preg is going to be impossible without changing the APIs in Drupal 6. I don't see that being likely.

My patch does exclude E_DEPRECATED errors, that works just fine.

Changing from ereg to preg is going to be impossible without changing the APIs in Drupal 6. I don't see that being likely.

I'm not doubting you, but could you give an example why the API would have to change? Why can't the calls just be rewritten to use preg equivalents?

http://api.drupal.org/api/function/drupal_system_listing/6

$mask is a ereg-style regex, that is a function parameter. Changing that to preg would break any module that would use that function. If we can't remove *all* calls to ereg_*, we can just leave all of them there, we need to hide E_DEPRECATED anyway.

Yeah, the ereg to preg changes cause a lot more code churn and entails a lot more risk. This patch does just enought to ensure PHP 5.3 compatibility and no more.

Function _user_categories($account) is passed $account. The following line uses $user. I changed $user to use $account and some error messages disappeared from the themes list.
if (function_exists($function) && ($data = $function('categories', $null, $user, ''))) {

So where does that put D7? Can ereg (and other issues which may cause E_DEPRECATED) be removed from D7? There doesn't seem to be an analogue for this issue for the Drupal 7 branch. Given that one of the big pushes of D7 is its correctness, I would assume that this would be the case.

-1 for three-year-old issue threads with misleading/inconspicuous titles. It looks like #64967: Replace ereg with preg is actually the relevant issue as far as ereg/preg is concerned. But I guess what I'm asking is, are there other E_DEPRECATED issues that have been spotted besides the ereg one?

Status:Reviewed & tested by the community» Needs work

As usual, the fixes have been commited to D7 first (in this very issue..), D7 is fully PHP 5.3 compatible.

#70 pointed out a error in this patch, I will re-roll this asap.

Status:Needs work» Needs review
StatusFileSize
new6.49 KB

Renamed wrong $user to $account.

5.3 is out and is the recommended standard right now; let's presume that the folks at PHP are as careful with what they do as we are and make sure that we deliver a version of Drupal which works on the currently shipping version of PHP. I have tested Drupal on 5.3 and while a few things break the solutions are not complicated.

On the ereg<->preg issue the translation of an ereg pattern into a preg one is rather straightforward, so instead of changing every upstream call to reformulate patterns as preg let's just transcode the pattern at execution time. If nobody else wants to tackel that one we'll assign someone from the team to do that. PHP 5.3.0 has performance advantages, and our sites will need those soon enough.

PHP 5.3 is now installed by default when you install the Macports PHP5 package on the Mac. It's causing some consternation among my newer coworkers.

Further evidence that we need to figure out how to get Drupal running cleanly on PHP5, whether or not said procedure is actually a good idea in production. Now that the PHP project has declared that PHP 5.3 is "released", it will continue to turn up all over the place and trip up many, many users.

Offtopic but handy: Those who need to install an older version of a Macports PHP5 package should consult this annoying procedure:
http://www.jesuscarrera.info/2009/02/28/downgrade-ruby-version-using-mac...

Priority:Critical» Normal
Status:Needs review» Needs work

<?php
+    $function = $module .'_user';
+   
// $null and $user need to be passed by reference.
+    if (function_exists($function) && ($data = $function('categories', $null, $account, ''))) {
?>

^ The comment says $user, the function says $account

<?php
+    $function = $module .'_user';
+   
// We can't use neither module_invoke nor user_module_invoke because we need the return value and by reference.
+    if (function_exists($function) && ($data = $function($hook, $edit, $account, $category))) {
+     
$groups = array_merge($data, $groups);
?>

^ Parse error near "by reference"? ;)

<?php
//// Only variables can be passed by reference workaround.
$null = NULL;
user_module_invoke('logout', $null, $user);
?>

^ Four slashes? ;)

Let's wait for a few minor releases of PHP 5.3 to call that "critical", can we?

Also: this chunk will make the "stable to dev and back" patch fail to apply:

<?php
-  if ($errno & (E_ALL)) {
+  if (
$errno & (E_ALL ^ E_DEPRECATED)) {
?>

Hopefully, that will make Gabor remember that we want this in stable versions:

<?php
-  if ($errno & (E_ALL ^ E_DEPRECATED)) {
+  if (
$errno & (E_ALL ^ E_NOTICE ^ E_DEPRECATED)) {
?>

#76 is working on PHP 5.3 D6.13 and using book, comment, database logging, help, menu, path, PHP filter, statistics, taxonomy, and update. I switched on aggregator, blog, and blog api and they worked. Can we at least put #76 through so that we have something working for commonly used core modules when the next security release of D6 goes out. We can then test the optional core modules from a stable base.

- I will fix the things pointed out by DamZ soon.. Seems I am unable to produce a proper patch :-/

- I *really* doubt that ereg/preg conversion will happen in D6, atleast not in this patch. The errors are hidden and ereg will not be removed before PHP6, so it should be fairly save for now.

- Core patches are only commited when they are working correctly. There are only some minor improvements left to do I think, it should work fine with all core modules.

I forgot to say that I'm -1 on any kind of hacks to remove ereg and friends. Those are still perfectly supported (even if deprecated) by PHP 5.3. There is really no point in spending any energy on that for Drupal 6. As noted already, Drupal 7 is already 100% ereg free (in compliance with PHP rules and regulations).

Status:Needs work» Needs review
StatusFileSize
new6.43 KB

Re-roll with some comment improvements...

Status:Needs review» Reviewed & tested by the community

I think we are good to go although I still think it's absolutely unfair that one hand we need to patch up old Drupals to support bleeding edge (are you nuts to use a .0 PHP release?) but even the newest Drupal can't require the same. My heart yearns for closures.

Issue tags:+call by reference

adding "call by reference" tag. We'll get a lot more of this for contributed modules.

yeah regardless of this getting committed , D6 contrib will be broken on 5.3...

@Slobodan (#99):
Noone is saying that Drupal will be 5.3-ready after this patch. It's just one step.

If you really want to go 5.3 and need help, please have a look into your apache error.log (if the error messages are not shown in your browser window) and report as much information as you can get.

A lot of the 5.3 errors in contributed modules can be fixed by patching.

Funky hack for call_user_func_array (here in theme.inc, line 615)

<?php
 
if (isset($info['function'])) {
   
// The theme call is a function.
   
if (class_exists('ReflectionFunction') && is_string($info['function']) && function_exists($info['function'])) {
     
$reflect_function = new ReflectionFunction($info['function']);
      foreach (
$reflect_function->getParameters() as $i_param => $param) {
        if (
$param->isPassedByReference()) {
         
$args[$i_param] = &$args[$i_param];
        }
      }
    }
   
$output = call_user_func_array($info['function'], $args);
  }
?>

Anyway, I think the PHP implementation of call_user_func_array stinks.

Guys, I would like to propose a more radical and complete solution to the call by reference problem. The currently proposed patches only solve some symptoms.

A typical problem is that different hook implementations can have different by-value / by-reference signatures. This especially applies to the more generic functions like module_invoke_all().

The idea is to introduce a function call_user_func_array_ref, which automatically checks which arguments need to be passed by reference. This function can then be used as a replacement for call_user_func_array.

Here's the code (maybe include it in common.inc, I don't care):

<?php
function call_user_func_array_ref($callback, &$args) {
  if (
class_exists('ReflectionFunction')) {
    if (
is_string($callback)) {
     
$reflect = new ReflectionFunction($callback);
    } else if (
is_array($callback)) {
      if (
is_object($callback[0])) {
       
$reflect = new ReflectionMethod(get_class($callback[0]), $callback[1]);
      } else if (
is_string($callback[0]) && class_exists($callback[0])) {
       
$reflect = new ReflectionMethod($callback[0], $callback[1]);
      }
    }
    if (!
is_object($reflect)) {
      return
null;
    }
   
$args_new = array();
   
$params = $reflect->getParameters();
   
// the keys don't need to be sorted..
   
foreach (array_keys($args) as $i => $key) {
      if (isset(
$params[$i]) && $params[$i]->isPassedByReference()) {
       
$args_new[$key] = &$args[$key];
      } else {
       
$args_new[$key] = $args[$key];
      }
    }
    return
call_user_func_array($callback, $args_new);
  } else {
    return
call_user_func_array($callback, $args);
  }
}
?>

I tested it with the following little script:

<?php
function foo($x, &$y) {
 
$x .= '*';
 
$y .= '*';
}
// cufa = call_user_func_array
echo '<pre>                       | cufa_ref   | cufa';
echo
"<br/>array('four', 'five')  | ";
$args = array('four', 'five');
call_user_func_array_ref('foo', $args);
echo
implode(' ', $args);
echo
' | ';
$args = array('four', 'five');
call_user_func_array('foo', $args);
echo
implode(' ', $args);
echo
'<br/>array(&$a, &$b)        | ';
$a = 'six';
$b = 'seven';
$args = array(&$a, &$b);
call_user_func_array_ref('foo', $args);
echo
"$a $b";
echo
' | ';
$a = 'six';
$b = 'seven';
$args = array(&$a, &$b);
call_user_func_array('foo', $args);
echo
"$a $b";
echo
'</pre>';
?>

Result (on my system):

                       | cufa_ref   | cufa
array('four', 'five')  | four five* | four five
array(&$a, &$b)        | six seven* | six* seven*

As you can see, call_user_func_array_ref always changes the second argument, not the first one, as implied by the function signature. The native call_user_func_array, on the other hand, does not care at all about the function signature.

@donquixote - this patch is supposed to be for 6.x, so it must to every extent possible maintain the current API. Also - I'm guessing the reflection calls in that code sample would be rather slow? Also, please attach large code samples rather than pasting inline.

Yeah really. Why not just override __call() and do it in there?

__call is for methods, not functions.

@pwolanin:
Introducing a new function does not change the API, except that it can theoretically cause name clashes with custom or contrib code. But usually that has not been a reason for not introducing a new function.

Now obviously we need to be careful where it is really safe to replace the native call_user_func_array with call_user_func_array_ref(). In some cases, I would say, call_user_func_array_ref is more robust than a custom-made patch, in others it is not. So, we would only use this function in those places where it makes sense: Mostly these will be the places where we don't know the function signature.

Performance: I don't know how costly the reflection stuff is. But if we have many calls to the same function, some caching (remember the signature info in a static array) could help.

Another problem is that the solution has no effect in PHP 4. This doesn't have to be a problem per se, but it means we need to test things in PHP 4 and PHP 5.2 and PHP 5.3.

#84 working for me as well (can finally access certain pages without white-screens), but still have ereg errors showing up, will try to set error_reporting elsewhere. Thanks Berdir, you're everywhere!

Priority:Normal» Critical

Okay, this is getting pathetic. "Wait for D7" isn't going to cut it at this point; we need PHP 5.3 support now. Who knows how many potential new Drupal users are going to unknowingly install the stable version of Drupal on the stable version of PHP, see all the errors, and then start posting on their blogs and mailing lists about what a piece of unusable crap Drupal is? I'm sure it's happened already.

I'll help. What needs to be done to get a D6.14 out the door with full PHP 5.3 compatibility? Code? Money? Sex? Drugs? I'm game. Let's do it.

Test this patch, especially on older versions so that we can be sure that it still works on old, but still supported PHP versions (PHP 4!).

Do you mean the patch in #84? I'm getting a failing hunk in includes/common.inc on line 590. I'm not going to attempt to fix it because, to tell the truth, I don't really get how bitwise operators work well enough to do so in this case (shame, shame). But I'll see what an installation with the other hunks applied can do.

Our company still has a few accounts with our crappy old host who still hasn't upgraded from PHP 4.3 or thereabouts. I'll give it a try there once I get it uploaded (they don't support SSH either - really classy outfit, these guys).

The patch is against the -dev version....

I don't want to be spamming, but:

For those who need an immediate solution for D6+contrib in PHP 5.3, this is what I did:
- put the reflection-based call_user_func_array_ref (see above) into my bootstrap.inc
- I think you still need to apply the above patch (I did it manually), especially the error reporting flags.
- Every time I see some message complaining about by-reference args in call_user_func_array, I simply change the call_user_func_array call to call_user_func_array_ref. (unless I can find a very obvious alternative solution).

So far this strategy works totally ok. No more by-ref errors appearing. Maybe it has a little cost in performance, but it can't be that much, compared to other bottlenecks.

I'll have to do the patching again when I want to upgrade the respective modules or core. That's the price. But, changing some call_user_func_array to call_user_func_array_ref is manageable.

call_user_func_array is one of the most called functions, especially if you have many modules. And Reflection is really slow, so this probably not just a little performance bottleneck. Just saying....

I wonder why it is slow? The information is all in memory...

http://stackoverflow.com/questions/294582/php-5-reflection-api-performance
sjon_hortensius says he gets +500% when replacing an empty static function call. Which is really not much, if you take into account that an empty function call should be awesome fast, and the reflection call naturally requires more than one function call, with more than one parameter.

But maybe this one would be faster, and more likely to produce the same result in 5.3 (and lower) as call_user_func_array did before 5.3:

<?php
function call_user_func_array_ref($callback, [&]$args) {
 
$args_new = array();
  foreach (
array_keys($args) as $i => $key) {
   
$args_new[$key] = &$args[$key];
  }
  return
call_user_func_array($callback, $args_new);
}
?>

EDIT:

I'm not sure if we should have $args to be passed by-ref. It would be faster in many cases, but can have side effects. Passing it by-value would mean one more array copy (arrays can be big in Drupal), but would be more reliable.

I think there should be both versions available.
Or, why not go all the way and provide all three versions? If benchmarks prove the reflection beast to be slow, we could try caching the signature (or the entire $reflect object) in a static array. Cache could be enabled / disabled with a "remember" argument. (just thinking)

The point is:
The native implementation of call_user_func_array sucks. It totally ignores the signature, and allows call-time pass-by-reference even if that is clearly disabled in PHP settings. Having an alternative could be a good thing also for future code.

I'm still working at testing PHP 4 compatibility with the patch. The problem is that a lot of these old crappy hosting accounts have MySQL 4.0.something (yeah, seriously), and I keep getting the error about MySQL 4.1.1 being required when I'm trying to install. I know I've gotten D6 to work with MySQL 4.0 before somehow, but I can't remember how and such hackery would defeat the purpose.

Some of these accounts are supposed to have MySQL 5.something on them, but I'm not sure which ones. I'll keep trying.

Okay, I finally got a patched dev version on to a server with PHP 4.4.4 and MySQL 5.0.26. I enabled all core modules and posted a node with a menu item and a custom path without running into a single error. That's not a very extensive test, I know, but everything seems solid.

If you can, please test image upload with automatic image resizing, as there is a change that needs to be tested. Thanks.

I gave my account a user picture, intentionally selecting an image larger than the maximum allowed resolution. It downscaled it with no problems. I'm not sure where else core resizes images, but if there's another place you want me to check, let me know.

Status:Reviewed & tested by the community» Active

Status:Active» Needs work

@XerraX: Please read the handbook page on Issues status values: http://drupal.org/node/156119

I'd really love to get the next Drupal 6 release out with PHP 5.3 support. While you argue about how to build an all-encompassing solution which would solve all of contribs at the same time, we might slip the next Drupal 6 release again not to say the Drupal 5 branch, which would also need these changes. I'd say we should solve the Drupal core issues in core and fix the contrib issues in the respective modules unless we find a core soution which people agree on. The reflection based solution did not seem to catch that many hearts.

Also, I'd kindly ask people to follow up on this issue if they have feedback on the proposed approaches or patches. Needing to cherry-pick the few relevant comments from all the chatter about workarounds and Mac setups does not help the cause too much.

Status:Needs work» Reviewed & tested by the community

The patch in 87 still applies to latest -dev for me? There are some offsets, but it works just fine.

After some time of playing around, I can confirm that the reflection thing is not a desirable solution, even though it does suppress the error messages. Most of the complaints by PHP 5.3 are justified, and should be fixed instead of hiding them away.

Hook implementations should respect the hook signature, and hooks with by-ref parameters should not be called with module_invoke or module_invoke_all.

Another problem in some contributed modules is object-valued by-reference parameters which are called by-value. In PHP 5.x there is no need to have these parameters being by-ref, but in PHP 4.x it is necessary, unfortunately. One example is views, see http://drupal.org/node/569724.

Quite frustrating that we still need to support PHP 4.x.

That said, I agree with #151.

Status:Reviewed & tested by the community» Needs review

Reviewing the patch in #84 by looking at the code:

1. On the file_save_upload change: not all file validation functions seem to take $file as a reference. Looks like of the ones I found in core, only the resolution validator modifies the data actually (resizes the image), so we do need to support this pass by reference. Although I don't know how it worked in PHP 4 at all, given that it was passed by copy before this patch. I've seen http://api.drupal.org/api/function/file_validate_extensions/6 and http://api.drupal.org/api/function/file_validate_size/6 do not take $file by reference, so would result in a deprecated call time pass by reference error. Maybe that is all cleared off by our error settings, and it's not that big of a deal.

2. The only other change I have review issues with is why array_merge_recursive() becomes array_merge() in user_register_validate(). That seems to be totally unrelated and not reasoned in the patch either.

1. "$array = array(&$param); call_user_func_array($function, $array)" and $function(&$param). The first one does *not* result in a call time pass by reference error, see http://php.net/call_user_func_array.

"Note: Referenced variables in param_arr are passed to the function by a reference, others are passed by a value. In other words, it does not depend on the function signature whether the parameter is passed by a value or by a reference. "

And yes, this is bad....

2. That is probably wrong...

Status:Needs review» Needs work

OK, let's get 2. fixed then.

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new6.83 KB

Re-rolled, only change is array_merge to array_merge_recursive.

Version:6.x-dev» 5.x-dev
Status:Reviewed & tested by the community» Patch (to be ported)

Thanks, committed to Drupal 6, moving down to Drupal 5.

Great, finally! :)

Uhm, is there really anyone crazy enough to use D5 together with PHP 5.3 ? :)

I guess you don't have much choice if you have different projects running on the same server, some of them with D5, some D6, some wordpress, Zend, whatever, and one of them wants the latest PHP version. And your budget does not allow upgrading to D6 or moving to a different server.

I wonder how many people still use PHP 4.x with D6.. thinking backwards feels odd. It would be so nice to have a Drupal that ditches PHP 4 support, but still works with all the contributed modules for D6.

@donquixote - that's called Pressflow - it's a not-really-a-fork which has MySQL and PHP5-specific optimizations for D6.

I wonder how many people still use PHP 4.x with D6.

We have a few sites doing this, unfortunately, as our old shared hosting company has never upgraded past 4.4.3 or so. And some of their servers are still running MySQL 5.0, too… It took a while, but I finally convinced my boss that we needed to start ditching those losers.

Anyway, glad to see the patch that this thread produced was accepted in D6.14. Good work, everyone.

Status:Patch (to be ported)» Needs review
StatusFileSize
new8.08 KB

Here is my patch for Drupal 5, rolled against today's dev.
I started with the D6 patch, #157 and manually applied all the patches that were relevant to D5. Then I fixed problems until I got no errors on any of my seven Drupal 5 sites. I might have missed something, but it works pretty well for me.

Status:Needs review» Needs work

Contains testing code and unnecessary changes to .info files on initial review.

Status:Needs work» Needs review
StatusFileSize
new5.62 KB

Removed testing code and changes to .info files.

StatusFileSize
new5.38 KB

removed unnecessary extra empty line

A Drupal 6.14 php 5.3 issue popped up for me with: drupal_retrieve_form

Here is a proposed patch;

diff -Nurp ../drupal-6.14.orig/includes/form.inc ./includes/form.inc
--- ../drupal-6.14.orig/includes/form.inc       2009-12-03 10:11:43.000000000 -0600
+++ ./includes/form.inc 2009-12-03 10:12:16.000000000 -0600
@@ -322,7 +322,7 @@ function drupal_execute($form_id, &$form
  *   these additional arguments and conditionally return different
  *   builder functions as well.
  */
-function drupal_retrieve_form($form_id, &$form_state) {
+function drupal_retrieve_form($form_id, $form_state) {
   static $forms;
   // We save two copies of the incoming arguments: one for modules to use

Please make sure that it is safe for me to remove the & at this location.

No, it's not. You don't need to change the function, you need to fix who is calling the function in a wrong way. See http://api.drupal.org/api/function/drupal_execute/6 for a correct example (The "$args[1] = &$form_state;" part is important).

You can use the following snippet to find out by whom the function is called:

<?php
if (is_null($form_state)) {
 
debug_print_backtrace();
  die();
}
?>

Place that directly at the top of drupal_retrieve_form and it will show the full backtrace once called incorrectly.

Hint: I've just checked again that all calls to that function are correctly implemented in core so it is either a contrib module or even custum code. If it is a contrib module, open a bug report if there isn't already one.

thanks, this helps out a lot I will look into finding the correct troublesome module.

Okay, so after further studying of drupal and the numerous modules I have had to make PHP 5.3 band-aids for, I have identified the following.

The problem seems to happen in the following case:
When call_user_func_array(..) is called, the $args array must get created.
Apparently, with php 5.3 when an $args gets created, one must explicitly set the reference inside of the $args array.

For example:

  $args = array(
    $node_form_id,
    $node_form_state,
    $node
  );

Is passed as the arguments to:
function some_function($id, &$state, $node){
..

Because parameter 2 is set as reference by the some_function(..), then the $args must have the second parameter passed as a reference:

  $args = array(
    $node_form_id,
    &$node_form_state,
    $node
  );

Does this sound correct?

If so, then the proper solution is indeed not removing the reference parameter with some_function(..) but instead by finding every instance of call_user_func_array('some_function', $args) and correct the $args array contents.

If some_function(..) never actually needs $state to be a reference, then the band-aid patches are probably simplest fix and are valid, otherwise the band-aid patches are dangerous and should be reverted.
(I am going to point any PHP 5.3 reference threads I participated in to this post.)

Have updated the http://drupal.org/requirements page.

The update on the requirements page may have been made in haste.

PHP 5.3 and Drupal 6.15 seem to continue to throw:
Deprecated: Function ereg() is deprecated in C:\wamp\www\mysite\includes\file.inc on line 902

http://drupal.org/node/689570#comment-2496144

I've confirmed this by trying to install 6.15 on WAMP with PHP 5.3

Unless you've overriden the default drupal error handler, those E_DEPRECATED errors shouldn't be displayed, no idea why they would show up for you.

I didn't override anything. It's a fresh install of both WAMP and Drupal 6.15.

The comment I liked to is also dated today. where the user installed WAMP then installed Drupal 6.15 and recieved the same using PHP 5.3 that ships with WAMP. I suppose it could be WAMP specific. Off to Ubuntu to test there.

Re-test of php53_warnings.patch from comment @comment was requested by matt.nz.

I'm seeing the same problem with a fresh 6.15 install on Solaris AMP using PHP 5.3.1. This is annoying since the Drupal Installation Requirements states that Drupal 6.14 and higher support PHP 5.3.

Ah I see the issue. It seems that the function that does use ereg() is called before the error handler from Drupal is set. You can "fix" it by disabling E_DEPRECATED errors in your php.ini or in a htaccess file.

Example:

error_reporting  =  E_ALL ^ E_DEPRECATED

@Berdir:
I really appreciate your (& the community) efforts on making this compatible. tones of thanks ;) subscribed!

Subscribing.

@ Berdir in #214 and #219: I can confirm this is a problem. There is a user on our local Drupal community site that has the same problem using PHP 5.3. Is the patch from #157 indeed the last one made for D6? I'll see if I can convert ereg() to preg_match().

A temporary fix for sites running 5.3 might be to put

// Temporary workaround for PHP 5.3 incompatibility - see http://drupal.org/node/360605.
error_reporting(E_ALL ^ E_DEPRECATED);

in settings.php.

The workaround in #219 will work on many sites but not for some sites running PHP as CGI on shared hosts.

A more permanent solution might be to move the conditional declaration of E_DEPRECATED from common.inc to bootstrap.inc and also early in the bootstrap to insert error_reporting(E_ALL ^ E_DEPRECATED);. This could probably go directly in http://api.drupal.org/api/function/_drupal_bootstrap/6 in the DRUPAL_BOOTSTRAP_CONFIGURATION phase, immediately after the timer_start(). The test for E_DEPRECATED in drupal_error_handler could probably then be removed.

A problem I can see - this does mean you can't change error_reporting in .htaccess or php.ini since it will be modified during the bootstrap. Is this an issue? If so then perhaps a simple error handler is needed just for the bootstrap that filters out the E_DEPRECATEDs, until we are fully bootstrapped and the main error handler is available. That could be set with set_error_handler('bootstrap_error_handler', E_DEPRECATED) (see http://php.net/set_error_handler).

The other option would be to set the full Drupal error handler early in bootstrap, which would also enable error handing during boostrap .. though in view of the iterations needed in #325169: Move error/exception handler higher up in the bootstrap process adding something so featureish at this stage to 6.x is probably an extremely bad idea.

- set_error_handler('bootstrap_error_handler', E_DEPRECATED) -> That's PHP 5 only, so not possible

- Agree, there is no way to backport 325169 to Drupal 6

- We could do something like this in bootstrap.inc

<?php
// Hide E_DEPRECATED messages.
if (defined('E_DEPRECATED')) {
 
error_reporting(error_reporting() ^ E_DEPRECATED);
}
?>

That would remove E_DEPRECATED from the error reporting setting but keep everything else as it has been set before.

We can leave the definition of E_DEPRECATED and error handling as it is with that change, I think.

Ah yes, that looks good.

> leave the definition of E_DEPRECATED and error handling as it is
I was forgetting that error_reporting only affects the built-in PHP error handling and that the custom error handler is called anyway regardless of the error_reporting setting. Well, that's my latest understanding - I find the docs could be a bit clearer on this :P

Status:Needs work» Needs review
StatusFileSize
new684 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch php53_remove_deprecated.patch.
[ View ]

The attached patch works correctly for me and hides the early DEPRECATED messages.

I apply patch but flowing errors array:
Deprecated: Function ereg() is deprecated in /users/drupal/www/includes/file.inc on line 902

Another Drupal 5.x specific problem is usage of parse_ini_file() and lack of double quotes in info files (see http://drupal.org/node/722430 and http://www.php.net/manual/en/function.parse-ini-file.php#93148).

Can someone please fix includes/files.inc: 911

Before:
elseif ($depth >= $min_depth && ereg($mask, $file)) {
After:
elseif ($depth >= $min_depth && preg_match('/' . $mask . '/', $file)) {

That *can not* be fixed, because $mask is a function parameter and changing ereg to preg_match would break compatibility.

From looking at how core 6.x and contrib modules actually use file_scan_directory(...), the only issue with converting ereg to preg_match would be the "/" character.

So we could add something like

<?php
elseif ($depth >= $min_depth && preg_match('/' . escapeSlashes($mask) . '/', $file)) {
...
...
function
escapeSlashes($s)
{
    return
str_replace('/', '\/', $s);
}
?>

The / is only used as a delimiter. Change the delimeter to something more esoteric and there's no problem. I was informed that this is a duplicate of http://drupal.org/node/530940 where there is a working patch that should meet the unit test. But it does not pass due to some flaw in the test itself probably since use of preg_match on the data type string in $mask returns the proper booleans.

This issue concerns a bug in Drupal. Please do not post support requests in this issue. While I understand the problems you are wrestling with, questions on how to patch, and questions about unrelated problems interfere with getting the fix in a future release.

If your followup isn't about a patch review, proposal or testresult of this issue, please do not post here, but see http://drupal.org/support for your support options instead.

I've unpublished the offtopic comments.

Version:6.16» 6.x-dev

Summary of the issue so far:

- A lot of fixes regarding references went into Drupal core
- Contrib modules may still have issues with PHP 5.3 - please file such issues in the issue queue of the offending module - followups about non-core modules will be removed.

Fresh core installation sometimes throw E_DEPRECATED warnings on ereg. The patch on #360605-233: PHP 5.3 Compatibility aims to solve this.

@ #259, #260 - please respond to #68 's.

$mask is a ereg-style regex, that is a function parameter. Changing that to preg would break any module that would use that function. If we can't remove *all* calls to ereg_*, we can just leave all of them there, we need to hide E_DEPRECATED anyway.

Heine: That sounds reasonable. Hopefully, I can review the patch in #233 soon. For PHP 5.3 compatibility with contrib modules, I might create a wiki page on Groups.Drupal.org, so that it can be tracked in a table.

Patch in #360605-233: PHP 5.3 Compatibility suppressed the E_DEPRECATED errors on a test installation (with error_reporting = E_ALL | E_STRICT). This install was unusable before the patch.

The patch could use additional comments (why here, why the define check).

@Aleksic, was the patch correctly applied? Where did you see the errors after the patch?

Apparantly, another summary is in order:

Contrib modules may still have issues with PHP 5.3 - please file such issues in the issue queue of the offending module - followups about non-core modules will be removed.

Drupal 6.16 itself has (AFAICT) one remaining issue on PHP 5.3 where E_DEPRECATED errors are thrown on ereg in file_scan_directory. The patch in #360605-233: PHP 5.3 Compatibility aims to solve this.

Any followup not related to fixing this issue, will be removed.

There is another bug, see #587568: Parameter 1 to comment_nodeapi() expected to be a reference module.inc, line 450 (php 5.3 compatibility). Patch there is basically just waiting to be RTBC'd...

subscribing

Version:6.x-dev» 5.x-dev

Reading the backscroll I noticed someone who was merely subscribing to this issue (annoyingly) set it from 5.x-dev to 6.x-dev (see comment #227). Gabor had changed this to 5.x-dev after committing a patch to the 6.x branch (comment #158), so I am changing the issue back to the correct/intended version number.

Version:5.x-dev» 6.x-dev

The ereg deprecated error _is_ still an issue on Drupal 6.x-dev.

@Heine: our error handler ignores E_DEPRECATED errors. The only way you can get those errors currently is by (1) having E_DEPRECATED enabled in php.ini (bad idea number 1), and (2) having display_errors enabled (bad idea number 2).

Berdir patch in #233 would fix this corner case, but it feels like an unnecessary protection against a server mis-configuration to me.

Status:Needs review» Reviewed & tested by the community

That said, given the impact on Drupal's image of those silly warnings, especially to new comers evaluating Drupal on WAMP distributions shipping with PHP 5.3, let's get #233 in.

Thanks for RTBC, yeah, it is not necessary but on many shared hosts, users probably don't have access to change display_errors/error_reporting setting (or they don't know how) so with the patch in #233 it should just work in these cases.

@Damien, thanks for the RTBC. What I get from reading the forum is that all these bad ideas happen, not just on localhost/wamp, but also on shared hosts.

Status:Reviewed & tested by the community» Fixed

Great, committed #233, marking as fixed.

Version:6.x-dev» 5.x-dev
Priority:Critical» Normal
Status:Fixed» Active

Just noticed that Drupal 5 does not work on PHP 5.3 either....

    * : Function ereg() is deprecated in /home/mati/apache/htdocs/drupal5/includes/file.inc on line 646.
    * : Function ereg() is deprecated in /home/mati/apache/htdocs/drupal5/includes/file.inc on line 646.
    * : Function ereg() is deprecated in /home/mati/apache/htdocs/drupal5/includes/file.inc on line 646.
    * : Function ereg() is deprecated in /home/mati/apache/htdocs/drupal5/includes/file.inc on line 646.
    * warning: syntax error, unexpected BOOL_TRUE in modules/comment/comment.info on line 3 in /home/mati/apache/htdocs/drupal5/includes/module.inc on line 195.
    * warning: syntax error, unexpected BOOL_TRUE in modules/drupal/drupal.info on line 3 in /home/mati/apache/htdocs/drupal5/includes/module.inc on line 195.
    * warning: syntax error, unexpected BOOL_TRUE in modules/node/node.info on line 3 in /home/mati/apache/htdocs/drupal5/includes/module.inc on line 195.
    * warning: syntax error, unexpected BOOL_TRUE in modules/poll/poll.info on line 3 in /home/mati/apache/htdocs/drupal5/includes/module.inc on line 195.

Also getting the following which breaks Drupal 5 completely on PHP 5.3...

    * warning: Parameter 3 to block_user() expected to be a reference, value given in /home/mati/apache/htdocs/drupal5/includes/module.inc on line 386.
    * warning: Parameter 3 to comment_user() expected to be a reference, value given in /home/mati/apache/htdocs/drupal5/includes/module.inc on line 386.
    * warning: Parameter 2 to node_user() expected to be a reference, value given in /home/mati/apache/htdocs/drupal5/includes/module.inc on line 386.
    * warning: Parameter 3 to system_user() expected to be a reference, value given in /home/mati/apache/htdocs/drupal5/includes/module.inc on line 386.
    * warning: Parameter 2 to user_user() expected to be a reference, value given in /home/mati/apache/htdocs/drupal5/includes/module.inc on line 386.
    * warning: Parameter 2 to watchdog_user() expected to be a reference, value given in /home/mati/apache/htdocs/drupal5/includes/module.inc on line 386.

Version:5.x-dev» 6.x-dev
Priority:Normal» Critical
Status:Active» Fixed

I don't know if we're going to be doing D5 PHP 5.3 compatibility work. That's up to Neil. But for the love of Druplicon can we make that a new thread before this one gets to a second page?

Guys, I hope I am not just adding to the clutter here...but PLEASE tell me I have no need to panic...
http://www.php.net/archive/2010.php#id2010-07-22-1
PHP drops 5.2x branch support, urges 5.3 upgrade pronto.
I checked with my host (Downtownhost.com), and they will roll out 5.3 very soon as part of standard upgrade procedure within cPanel...which just happens to be the same software every cat and their dog in Web hosting land uses...meaning much of the shared hosting sites are about to have a problem with D6+D5 all over teh interwebs.

...ok, panic now....

@Onyx: if you are on 6.17 (or even 6.16, I think), no need to panic. I think that #853064: Alter INSTALL.txt to be clear PHP 5.3 is not compatible with Drupal 5 for 5.x should be resolved soon, but in light of D7's impending release, people should be preparing to upgrade from D5 anyway.

Can someone lock this thread?

Further PHP 5.3 issues should be resolved either in the Drupal 5 thread mentioned above or in individual issues for Drupal 6, Drupal 7, or contrib modules as appropriate. Locking this thread for posterity.

Status:Fixed» Closed (fixed)
Issue tags:-PHP, -PHP 5.3, -call by reference, -ereg deprecation

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