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.
Comment | File | Size | Author |
---|---|---|---|
#233 | php53_remove_deprecated.patch | 684 bytes | Berdir |
#176 | drupal.php53_compat_03-D5.patch | 5.38 KB | andrewfn |
#175 | drupal.php53_compat_02-D5.patch | 5.62 KB | andrewfn |
#173 | drupal.php53_compat_01-D5.patch | 8.08 KB | andrewfn |
#157 | drupal.php53_compat_10-D6.patch | 6.83 KB | Berdir |
Comments
Comment #1
chx CreditAttribution: chx commentedI 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.
Comment #2
chx CreditAttribution: chx commentedOh and thanks a ton for working on this!
Comment #3
chx CreditAttribution: chx commentedThanks 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.
Comment #4
Dave ReidYay references! Following to watch progress.
Comment #5
catchBumping 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.
Comment #6
BerdirSome 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:
I tried to find a function with an unchanged warning but had no luck so far.
Comment #7
BerdirRe-setting priority to critical.
Comment #8
Dave ReidMarked #253023: status report, etc. broken under php 5.3 as a duplicate of this issue.
Comment #9
BerdirOk, 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.
1337... this *has* to be good :)
Comment #10
Damien Tournoud CreditAttribution: Damien Tournoud commentedWould be better with a patch :)
Comment #11
BerdirForgot the patch...
Comment #12
Dries CreditAttribution: Dries commentedThis patch looks good to me. Will commit after another review.
Comment #13
Damien Tournoud CreditAttribution: Damien Tournoud commentedTwo nitpicks:
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?
This sentence should end with a dot.
Comment #14
Berdir> 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:
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.
Comment #15
BerdirOk, 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
Comment #16
Dries CreditAttribution: Dries commentedWorks! Committed to CVS HEAD.
Comment #17
chx CreditAttribution: chx commentedComment #18
BerdirMaking 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:
( 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:
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.
Comment #19
BerdirForgot to set the right status
Comment #20
catchIt 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.
Comment #21
BerdirPHP 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.
Comment #22
BerdirOk, 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 :)
Comment #23
moshe weitzman CreditAttribution: moshe weitzman commentedTypo 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.
Comment #24
BerdirI 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.
Comment #25
Berdir- 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 :)
Comment #26
penguins CreditAttribution: penguins commentedIs it possible to apply the patch to my current Drupal 6.10. At the moment I'm having trouble editing user profiles.
Comment #27
Berdir@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.
Comment #28
Mike Wacker CreditAttribution: Mike Wacker commentedSo 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
Comment #29
penguins CreditAttribution: penguins commentedThank 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.
Comment #30
Berdir@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.
Comment #31
quikone CreditAttribution: quikone commentedI am sure this is a dumb question but I won't know unless I ask it. How does one apply this patch?
Comment #32
Berdir@31
See: http://drupal.org/patch/apply
Comment #33
pwolanin CreditAttribution: pwolanin commentedpatch need to be unified diff format
Comment #34
BerdirOh, updated patch...
Comment #35
pwolanin CreditAttribution: pwolanin commentedwhy 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:
and
Also, typically if the regex contains '/' we use a different delimiter such as '@':
Comment #36
BerdirThe 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..
Comment #37
BerdirSetting to needs review again..
Comment #38
pwolanin CreditAttribution: pwolanin commentedlooks better, but this comment needs some work:
Comment #39
killes@www.drop.org CreditAttribution: killes@www.drop.org commentedParameter 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.
Comment #40
Berdir@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.
Comment #41
Gerhard Killesreiter CreditAttribution: Gerhard Killesreiter commentedyes, 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.
Comment #42
penguins CreditAttribution: penguins commentedDoes 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.
Comment #43
gr00vus CreditAttribution: gr00vus commentedHi,
Installing the patch I get the following:
Here are the contents of common.inc.rej:
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.
Comment #44
penguins CreditAttribution: penguins commentedTry 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.
Comment #45
BerdirThe 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.
Comment #46
gr00vus CreditAttribution: gr00vus commentedChanging 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:
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:
Thanks again.
Comment #47
penguins CreditAttribution: penguins commentedgr00vus,
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.
Comment #48
penguins CreditAttribution: penguins commentedBerdir,
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.
Comment #49
BerdirThanks for your tests..
I can't reproduce this. Maybe something is wrong with your write permissions?
Confirmed and fixed.
Again, I can't reproduce this.
This was the same bug as above.
@killes
I found your bug I think.
Or maybe another module... This is not a bug in core. That should be something like
Comment #50
ludovicofischer CreditAttribution: ludovicofischer commentedPart 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.
Comment #51
BerdirThat is the difference between a stable and a -dev release, dev releases display E_NOTICE messages.
Comment #52
gr00vus CreditAttribution: gr00vus commented@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.
Comment #53
penguins CreditAttribution: penguins commentedBerdir,
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.
Comment #54
penguins CreditAttribution: penguins commentedgr00vus,
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).
Comment #55
BerdirJust 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.
Comment #56
chx CreditAttribution: chx commentedI 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.
Comment #57
moshe weitzman CreditAttribution: moshe weitzman commented@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.
Comment #58
Mike Wacker CreditAttribution: Mike Wacker commentedOn 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.
Comment #59
chx CreditAttribution: chx commentedpeople 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.
Comment #60
shrop CreditAttribution: shrop commentedsubscribe
Comment #61
clintthayer CreditAttribution: clintthayer commentedAnyone have any thoughts on Drupal 5 and php 5.3?
Any thoughts would be helpful.
Comment #62
Mike Wacker CreditAttribution: Mike Wacker commentedI 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.
Comment #63
chx CreditAttribution: chx commentedBerdir says the latest patch does away with the hook_user API break. Aye. So let's go then.
Comment #64
Garrett Albright CreditAttribution: Garrett Albright commentedPHP 5.3 deprecates the ereg library, which appears to be in use in a disturbing number of places in core:
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.
Comment #65
Dave ReidChanging from ereg to preg is going to be impossible without changing the APIs in Drupal 6. I don't see that being likely.
Comment #66
BerdirMy patch does exclude E_DEPRECATED errors, that works just fine.
Comment #67
Garrett Albright CreditAttribution: Garrett Albright commentedI'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?
Comment #68
Berdirhttp://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.
Comment #69
Mike Wacker CreditAttribution: Mike Wacker commentedYeah, 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.
Comment #70
peterx CreditAttribution: peterx commentedFunction
_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, ''))) {
Comment #71
christefano CreditAttribution: christefano commentedI wish I'd seen this before I upgraded a site to a 5.3 RC a few months ago. Looking forward to mysqlnd. Subscribing.
Comment #72
Garrett Albright CreditAttribution: Garrett Albright commentedSo 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.
Comment #73
Dave Reid@Garrett: Um, ereg has already been removed from Drupal 7.
http://drupal.org/node/224333#file_scan_directory_nomatch\
#74645: modify file_scan_directory to include a regex for the nomask.
Comment #74
Garrett Albright CreditAttribution: Garrett Albright commented-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?
Comment #75
BerdirAs 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.
Comment #76
BerdirRenamed wrong $user to $account.
Comment #77
MisterSpeed CreditAttribution: MisterSpeed commented5.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.
Comment #78
mike booth CreditAttribution: mike booth commentedPHP 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...
Comment #79
Damien Tournoud CreditAttribution: Damien Tournoud commented^ The comment says $user, the function says $account
^ Parse error near "by reference"? ;)
^ Four slashes? ;)
Let's wait for a few minor releases of PHP 5.3 to call that "critical", can we?
Comment #80
Damien Tournoud CreditAttribution: Damien Tournoud commentedAlso: this chunk will make the "stable to dev and back" patch fail to apply:
Hopefully, that will make Gabor remember that we want this in stable versions:
Comment #81
peterx CreditAttribution: peterx commented#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.
Comment #82
Berdir- 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.
Comment #83
Damien Tournoud CreditAttribution: Damien Tournoud commentedI 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).
Comment #84
BerdirRe-roll with some comment improvements...
Comment #88
ngr CreditAttribution: ngr commentedI recently got drupal running on my computer, but in Administer, the status report link takes me to a blank page.
When I go to "Reports" I get an error message:
"warning: Parameter 1 to theme_status_report() expected to be a reference, value given in C:\wamp\www\Drupal\includes\theme.inc on line 617."
My question is if with the first path that you attached I can fix it. And the second question is if I can fix the warning with your path, where I have to put it.
thank you
Comment #89
BerdirThis is fixed with my latest patch. See http://drupal.org/patch/apply on how to apply patches.
Comment #90
johnthomas00 CreditAttribution: johnthomas00 commentedSubscribe +1
Will the next Drupal-6.x release include this fix?
Comment #91
febbraro CreditAttribution: febbraro commentedSubscribe.
FYI the Remi repo for CentOS put PHP 5.3 live over the weekend so now all servers that autoupdate packages will roll to 5.3 automatically and be greeted with a whole slew of "notices", great way to start a week.
Comment #92
eric02138 CreditAttribution: eric02138 commentedYup - Remi got me, too.
Comment #93
peterx CreditAttribution: peterx commentedOne of my test VPSs had 5.3.0 appear in WHM for a couple of days then the ISP removed the option because of the megazillion questions from people who chose to upgrade productions sites to 5.3.0 without testing. Not just Drupal sites. If Drupal 6 was the first major CMS compliant with 5.3.0 then there would be a lot of ISPs recommending an upgrade from X to Drupal.
Comment #94
chx CreditAttribution: chx commentedI 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.
Comment #95
donquixote CreditAttribution: donquixote commentedOn my machine I need PHP 5.3, otherwise I get apache crashes caused by php_mysqli.dll
Comment #96
roball CreditAttribution: roball commentedI also tried
on a CentOS 5.3 server, but had to revert back to PHP 5.2.10, since Drupal 6.13 didn't work properly on PHP 5.3.0. Looots of problems...
Comment #97
donquixote CreditAttribution: donquixote commentedadding "call by reference" tag. We'll get a lot more of this for contributed modules.
Comment #98
chx CreditAttribution: chx commentedyeah regardless of this getting committed , D6 contrib will be broken on 5.3...
Comment #99
Aleksic CreditAttribution: Aleksic commentedHi good people,
I was apply patch but still I`m unable to view edit/account...
Can someone help me please?!
Thanks
Comment #100
donquixote CreditAttribution: donquixote commented@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.
Comment #101
agerson CreditAttribution: agerson commentedSubscribing... Getting the function ereg() is deprecated in drupal \includes\file.inc on line 895 error.
Comment #102
Ammermag CreditAttribution: Ammermag commentedFYI: Macport does not have a PHP 5.2.x version available. After looking at many blogs and tips, the best way to downgrade my Macbook pro/Mac OS X Leopard back to a PHP 5.2.x was to get version 5.2.9 from http://www.entropy.ch/software/macosx/php/. Installation is easy as it uses a standard Mac package to install. No compile is required. This is good if you are only worried about getting source for the latest Mac OS x.
Comment #103
Garrett Albright CreditAttribution: Garrett Albright commentedSupposedly there's a way to install older versions of MacPorts ports using the techniques on this page, and they even use PHP 5.2 in an example at the very bottom of the page. I couldn't get it to work, though; I'm trying to upgrade from PHP 5.2.8 to 5.2.10, but it keeps trying to download the PHP 5.3.0 tarball, then bailing after the hashes mismatch - I guess it's comparing the hashes of the 5.3.0 tarball against 5.2.10's known hashes.
However, so long as you haven't deleted deactivated ports on your machine, it should be fairly easy to downgrade to a previously-installed version of a port. Run
port installed php5
and you'll see all the versions of the PHP 5 port you have installed, with one of them marked as active. If you see a previous 5.2 version there you'd like to downgrade to, try something like:You'll want to run it as root, obviously, and the version numbers may differ depending on what you have installed. If all else fails, copy and paste the port and version from the output of
port installed
.MacPorts could definitely stand to improve in the realms of ignoring particular port versions and easily installing older versions of ports.
Comment #104
pescetti CreditAttribution: pescetti commentedsubscribing
Comment #105
Cyberwolf CreditAttribution: Cyberwolf commentedsubscribing
Comment #106
donquixote CreditAttribution: donquixote commentedFunky hack for call_user_func_array (here in theme.inc, line 615)
Anyway, I think the PHP implementation of call_user_func_array stinks.
Comment #107
AkkuDreamz CreditAttribution: AkkuDreamz commentedany update on this from the drupal team?
when are we going to get a PHP 5.3 compatible drupal.
Comment #108
AdrianB CreditAttribution: AdrianB commentedSubscribing
XAMPP now ships with 5.3.0 and it's quite popular as a local development platform. Even if you don't use 5.3 on production server you may want to run the same code local using XAMPP.
The problem with D6 and 5.3 has led to issues like this: http://www.apachefriends.org/f/viewtopic.php?f=29&t=36766
Comment #109
BMDan CreditAttribution: BMDan commentedTo resolve the macports issue, I found a good reference at http://bangpound.org/node/7548
To downgrade a box with RPM as a package manager, download all relevant files into a new directory, cd into it, and run:
rpm -Uvh $(rpm -qa --qf '%{NAME} %{VERSION} %{ARCH}\n' | awk '/^php.* 5\.3/ {print $1"*"$3".rpm"}')
Once you see no errors except for version-downgrade ones, just change the line to read:
rpm -Uvh $(rpm -qa --qf '%{NAME} %{VERSION} %{ARCH}\n' | awk '/^php.* 5\.3/ {print $1"*"$3".rpm"}') --oldpackage
Standard disclaimers: Run this code at your own risk, YMMV, not responsible for missing cats, etc.
Comment #110
Garrett Albright CreditAttribution: Garrett Albright commentedHmm, those steps at the bangpound site seemed to work. Sweet, thanks! I had heard about the local repository solution, but figured it was going to be more difficult than that.
However, the steps about creating a local repository on the Macports site seem to be a little out of date. I added the local repository above the rsync one in the sources.conf file as per the instructions, but it could never find my PHP package when I ran
portindex
. It seems you also need to move that little[default]
tag from after the rsync one to after the local one. So my sources.conf looks like this:After doing that (and downloading and downgrading the PHP package and
touch
ing the Portfile), MacPorts was finally able to see the PHP 5.2.10 package after runningportindex
and I was finally able tosudo port upgrade outdated
with impunity. Ah, it's been a while… looks like I'll be compiling all morning!Comment #111
roball CreditAttribution: roball commentedIf you are on RHEL or CentOS, reverting from 5.3.0 to 5.2.10 worked for me as follows:
Ensure that '/etc/yum.conf' contains the line
Then run
Comment #112
donquixote CreditAttribution: donquixote commentedGuys, 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):
I tested it with the following little script:
Result (on my system):
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.
Comment #113
Boobaasubscribing
Comment #114
joep.hendrix CreditAttribution: joep.hendrix commentedsubscribing
Comment #115
pwolanin CreditAttribution: pwolanin commented@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.
Comment #116
lucifurious CreditAttribution: lucifurious commentedYeah really. Why not just override __call() and do it in there?
Comment #117
donquixote CreditAttribution: donquixote commented__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.
Comment #118
frankcarey CreditAttribution: frankcarey commented#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!
Comment #119
AkkuDreamz CreditAttribution: AkkuDreamz commentedsubscribing
Comment #120
thinkyhead CreditAttribution: thinkyhead commentedPHP 5.3.0 is part of the standard install with Mac OS X 10.6 Snow Leopard, which lots of us are certain to be using for our Drupal development. It would be really great if all the deprecated functions and other errors could be patched in the next release of Drupal 5 and 6. Of course, there are issues with 3rd party modules and themes, including Zen, but most of the errors by far are in Drupal core.
Comment #121
chicagomom CreditAttribution: chicagomom commentedFYI, @windows.php.net/download/: "If you are using PHP with IIS you should use the VC9 versions of PHP". 5.3 is the first VC9 version.
Comment #122
stewart.adam CreditAttribution: stewart.adam commentedsubscribing
Comment #123
GrimSage CreditAttribution: GrimSage commentedsubscribing
Comment #124
jrabeemer CreditAttribution: jrabeemer commentedFor those who upgraded to Snow Leopard MacOS 10.6, it's now bundled, by default, with PHP 5.3.
If you plan to upgrade your Mac, you may want to hold off. I get tons of ereg error messages with the latest Acquia Drupal.
Comment #125
Garrett Albright CreditAttribution: Garrett Albright commentedOkay, 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.
Comment #126
BerdirTest 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!).
Comment #127
Garrett Albright CreditAttribution: Garrett Albright commentedDo 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).
Comment #128
BerdirThe patch is against the -dev version....
Comment #129
jrabeemer CreditAttribution: jrabeemer commented/off topic
FYI, if you're on Snow Leopard and need to get some work done in the meantime, you can install the latest MAMP (w/PHP 5.2.6), set your webroot and port 80, copy your databases over and it will work.
I shut down my built-in Apache in the meantime until this patch gets committed.
Comment #130
donquixote CreditAttribution: donquixote commentedI 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.
Comment #131
Berdircall_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....
Comment #132
donquixote CreditAttribution: donquixote commentedI 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:
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.
Comment #133
Garrett Albright CreditAttribution: Garrett Albright commentedI'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.
Comment #134
Garrett Albright CreditAttribution: Garrett Albright commentedOkay, 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.
Comment #135
BerdirIf you can, please test image upload with automatic image resizing, as there is a change that needs to be tested. Thanks.
Comment #136
Garrett Albright CreditAttribution: Garrett Albright commentedI 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.
Comment #137
elgreg CreditAttribution: elgreg commentedFor those also having snow leopard difficulties - the quickest solution for me was to just grab the mac port of PHP 5.2.10 and then change the apache config to point to the old one - that way I can always switch to php 5.3 if I need it. To wit:
In terminal (and with macports already installed - which you probably have to redo if you switched to Snow Leopard and haven't already):
shut down apache
Then install php5.2.10 with the apache and mysql libs
Then update my Apple shipped apache httpd.conf file to switch the location of the loaded php module
Then restart apache
Hope someone besides me finds this useful. :)
Comment #138
Garrett Albright CreditAttribution: Garrett Albright commentedSweet, it looks like that php52 package is new as of just a few days ago. Thanks for pointing it out, elgreg.
Comment #139
sender CreditAttribution: sender commentedsubscribing
Comment #140
jpdaley CreditAttribution: jpdaley commentedThanks elgreg. Your instructions worked perfectly.
I just commented out the original LoadModule line, and added the new one. Now by simply switching which line is commented out I can go back and forth between php5.3 to php5.2. Works like a charm!
One note: I already had a MySql installation going, so I had to reconnect the socket.
My socket was running in /tmp/mysql.sock
I made a new link in the /opt/local/var/run/mysql5 directory to point to my original socket and all worked well.
In terminal make the appropriate directories for /opt/local/var/run/mysql5 using sudo mkdir.
then:
cd /opt/local/var/run/mysql5
sudo ln -s /tmp/mysql.sock mysqld.sock
replace /tmp/mysql.sock with whatever your original socket was.
Thanks again elgreg!
Comment #141
Garrett Albright CreditAttribution: Garrett Albright commentedLinking isn't necessary - you should be able to change the location of where the MySQL and MySQLi modules looks for your socket in the php.ini file. MacPorts puts it at /opt/local/etc.php.ini. Look for mysql.default_socket and mysqli.default_socket.
Comment #142
bjcool CreditAttribution: bjcool commentedsubscribing
Comment #143
elgreg CreditAttribution: elgreg commentedAwesome - glad you found it handy. I may just need to figure out why I _didn't_ need to change my socket. :)
Comment #144
Anonymous (not verified) CreditAttribution: Anonymous commentedtracking
Comment #145
drubage CreditAttribution: drubage commentedSubscribing... PLEASE a patch for this that actually fixes all problems would be great, PHP 5.3 seems fast so we'd like to take advantage!
Comment #146
toddgeist CreditAttribution: toddgeist commentedsubscribing
Comment #147
Aleksic CreditAttribution: Aleksic commentedsubscribing
Comment #148
XerraX CreditAttribution: XerraX commentedneed a patch for latest dev
Comment #149
XerraX CreditAttribution: XerraX commentedComment #150
Dave Reid@XerraX: Please read the handbook page on Issues status values: http://drupal.org/node/156119
Comment #151
Gábor HojtsyI'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.
Comment #152
BerdirThe patch in 87 still applies to latest -dev for me? There are some offsets, but it works just fine.
Comment #153
donquixote CreditAttribution: donquixote commentedAfter 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.
Comment #154
Gábor HojtsyReviewing 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.
Comment #155
Berdir1. "$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...
Comment #156
Gábor HojtsyOK, let's get 2. fixed then.
Comment #157
BerdirRe-rolled, only change is array_merge to array_merge_recursive.
Comment #158
Gábor HojtsyThanks, committed to Drupal 6, moving down to Drupal 5.
Comment #159
BerdirGreat, finally! :)
Uhm, is there really anyone crazy enough to use D5 together with PHP 5.3 ? :)
Comment #160
donquixote CreditAttribution: donquixote commentedI 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.
Comment #161
catch@donquixote - that's called Pressflow - it's a not-really-a-fork which has MySQL and PHP5-specific optimizations for D6.
Comment #162
hass CreditAttribution: hass commented+
Comment #163
Garrett Albright CreditAttribution: Garrett Albright commentedWe 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.
Comment #173
andrewfn CreditAttribution: andrewfn commentedHere 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.
Comment #174
Dave ReidContains testing code and unnecessary changes to .info files on initial review.
Comment #175
andrewfn CreditAttribution: andrewfn commentedRemoved testing code and changes to .info files.
Comment #176
andrewfn CreditAttribution: andrewfn commentedremoved unnecessary extra empty line
Comment #197
thekevinday CreditAttribution: thekevinday commentedA Drupal 6.14 php 5.3 issue popped up for me with: drupal_retrieve_form
Here is a proposed patch;
Please make sure that it is safe for me to remove the & at this location.
Comment #198
BerdirNo, 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:
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.
Comment #199
thekevinday CreditAttribution: thekevinday commentedthanks, this helps out a lot I will look into finding the correct troublesome module.
Comment #200
thekevinday CreditAttribution: thekevinday commentedOkay, 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:
Is passed as the arguments to:
Because parameter 2 is set as reference by the some_function(..), then the $args must have the second parameter passed as a reference:
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.)
Comment #212
gpk CreditAttribution: gpk commentedHave updated the http://drupal.org/requirements page.
Comment #213
VM CreditAttribution: VM commentedThe 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
Comment #214
BerdirUnless 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.
Comment #215
VM CreditAttribution: VM commentedI 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.
Comment #218
jrwjrw CreditAttribution: jrwjrw commentedI'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.
Comment #219
BerdirAh 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:
Comment #227
sepla CreditAttribution: sepla commented@Berdir:
I really appreciate your (& the community) efforts on making this compatible. tones of thanks ;) subscribed!
Comment #228
XanoSubscribing.
@ 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().
Comment #229
gpk CreditAttribution: gpk commentedA temporary fix for sites running 5.3 might be to put
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.
Comment #230
Berdir- 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
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.
Comment #231
gpk CreditAttribution: gpk commentedAh 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
Comment #233
BerdirThe attached patch works correctly for me and hides the early DEPRECATED messages.
Comment #234
Aleksic CreditAttribution: Aleksic commentedI apply patch but flowing errors array:
Deprecated: Function ereg() is deprecated in /users/drupal/www/includes/file.inc on line 902
Comment #240
hass CreditAttribution: hass commented#679404: Warning: Parameter 1 to profile_load_profile() expected to be a reference, value given in module_invoke() looks like another php 5.3 bug
Comment #242
tacituseu CreditAttribution: tacituseu commentedAnother 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).
Comment #257
fizk CreditAttribution: fizk commentedCan someone please fix includes/files.inc: 911
Before:
elseif ($depth >= $min_depth && ereg($mask, $file)) {
After:
elseif ($depth >= $min_depth && preg_match('/' . $mask . '/', $file)) {
Comment #258
BerdirThat *can not* be fixed, because $mask is a function parameter and changing ereg to preg_match would break compatibility.
Comment #259
fizk CreditAttribution: fizk commentedFrom 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
Comment #260
carlmcdade CreditAttribution: carlmcdade commentedThe / 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.
Comment #265
Heine CreditAttribution: Heine commentedThis 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.
Comment #266
Heine CreditAttribution: Heine commentedSummary 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.
Comment #267
EvanDonovan CreditAttribution: EvanDonovan commentedHeine: 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.
Comment #272
Heine CreditAttribution: Heine commentedPatch 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?
Comment #274
Heine CreditAttribution: Heine commentedApparantly, 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.
Comment #275
BerdirThere 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...
Comment #283
NancyDrusubscribing
Comment #286
calebgilbert CreditAttribution: calebgilbert commentedReading 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.
Comment #287
Heine CreditAttribution: Heine commentedThe ereg deprecated error _is_ still an issue on Drupal 6.x-dev.
Comment #288
Damien Tournoud CreditAttribution: Damien Tournoud commented@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) havingdisplay_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.
Comment #289
Damien Tournoud CreditAttribution: Damien Tournoud commentedThat 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.
Comment #290
BerdirThanks 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.
Comment #291
Heine CreditAttribution: Heine commented@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.
Comment #296
Gábor HojtsyGreat, committed #233, marking as fixed.
Comment #297
RobLoachJust noticed that Drupal 5 does not work on PHP 5.3 either....
Also getting the following which breaks Drupal 5 completely on PHP 5.3...
Comment #298
Crell CreditAttribution: Crell commentedI 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?
Comment #299
RobLoachDeal! #853064: Alter INSTALL.txt to be clear PHP 5.3 is not compatible with Drupal 5
Comment #302
onyxnz CreditAttribution: onyxnz commentedGuys, 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....
Comment #303
EvanDonovan CreditAttribution: EvanDonovan commented@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?
Comment #304
Crell CreditAttribution: Crell commentedFurther 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.