Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
PHP 5 has been released. It has much stricter error checking and helps us find some lazily coded lines. There are a few errors to be fixed in Drupal core. I'll upload patches as I fix errors. The most problematic file is xtemplate.inc which has a lot of errors. Is thei file in development elsewhere?
Comment | File | Size | Author |
---|---|---|---|
#46 | drupal_61 | 819 bytes | killes@www.drop.org |
#43 | system_3.patch | 1.3 KB | killes@www.drop.org |
#42 | system_2.patch | 587 bytes | killes@www.drop.org |
#41 | system_1.patch | 577 bytes | killes@www.drop.org |
#38 | taxo_1.patch | 752 bytes | killes@www.drop.org |
Comments
Comment #1
killes@www.drop.org CreditAttribution: killes@www.drop.org commentedOk, here is the first round of patches. redLED provided the neccessary hard- and software to get this done. Kjartan provided insight into StdClass.
Comment #2
(not verified) CreditAttribution: commentedIt would appear that xtmpl (the actual templating class the xtemplate theme uses, available at http://sourceforge.net/projects/xtpl/) is no longer being developed, with last cvs commit dating to 2001/09/19; which is the one we use. Thus, there is little hope on getting this addressed in any productive way short of doing it ourselves.
The following two messages include diffs which enable xtemplate to run on PHP 5 with no warnings; they do so by declaring all of xtmpl's functions and variables as public, which should be identical to the original functionality.
Unfortunately, I belive the result would work on PHP 5 exclusively.
- redLED
Comment #3
(not verified) CreditAttribution: commentedxtemplate.inc diff
Comment #4
(not verified) CreditAttribution: commentedxtemplate.theme diff
Comment #5
killes@www.drop.org CreditAttribution: killes@www.drop.org commentedAnother patch for forum.module
Comment #6
killes@www.drop.org CreditAttribution: killes@www.drop.org commentedUpdated forum patch
Comment #7
killes@www.drop.org CreditAttribution: killes@www.drop.org commentedA small patch for comment.module
Comment #8
killes@www.drop.org CreditAttribution: killes@www.drop.org commentedAnother one, that was forgotten, applies after all the others.
Comment #9
Steven CreditAttribution: Steven commentedOne of the problems is the change of behaviour of array_merge() in PHP5. In PHP4, you can pass non-array arguments to array_merge, which is the same as wrapping the parameter in array:
array_merge('a', 'b') is the same as array_merge(array('a'), array('b'))
This patch breaks this behaviour and simply assumes that the arguments are always arrays. This is not always the case.
For example in menu_get_active_help():
I'm going to look for more instances and see if it's worth it to include an array_merge wrapper. Something like this:
(note: the code is a bit ugly for speed reasons and optimal usage of references)
Comment #10
Steven CreditAttribution: Steven commentedSorry, I used regular PHP tags rather than square bracketed ones. I got rid of the funky code as well:
Comment #11
killes@www.drop.org CreditAttribution: killes@www.drop.org commentedI think it is a Really Bad Idea(tm) to emulate the old (broken) behaviour by a wrapper. I'd prefer to make module_invke return arrays averywhere. This woul dalso be more consistent.
Comment #12
Steven CreditAttribution: Steven commentedI don't think that's a good idea. Module_invoke_all is really an odd case: it simply throws together all arrays returned by the hooks without regard for duplicate elements. This is handy in several cases (_xmlrpc, _link) but annoying for others that do not return arrays.
Having the _help hook return single-element arrays doesn't make much sense. Having all hooks return arrays is madness.
The wrapper would only be used in those cases where we know we can get non-array arguments, not everywhere.
Comment #13
JonBob CreditAttribution: JonBob commentedI could go either way on this. It clearly is crazy to require an array return value for every hook. We currently have a few possible situations:
1) Hooks that are only called via module_invoke(), and so array_merge() never comes into the picture.
2) Hooks that already return arrays, so module_invoke_all() has no problem.
3) Hooks that return no value, called via module_invoke_all().
4) Hooks that could return arrays or scalars, and which rely on the PHP 4 array_merge() semantics to end up with an array.
A quick scan through the hook API reference seems to indicate that only hook_help() falls into that last category, so that would be the biggest change to make to be able to go along with killes' suggestion. I would object to any resolution that required a change to hooks in category 1 or 3; we'd need to account for no-return-value hooks somehow if module_invoke_all were to presume array return values.
On the other hand, I don't think explicitly checking for scalars and making them arrays is that bad of a solution. My gut feeling is that it would be better to do this directly inside module_invoke_all() rather than in an array_merge() wrapper though, because it would force us to really examine each case of array_merge() and come up with the best solution rather than adding unnecessary checks everywhere.
Comment #14
Steven CreditAttribution: Steven commentedI'm fine with Jonbob's suggestion then. The wrapper I suggested was just a way to reduce code duplication (it wouldn't be replacing array_merge everywhere).
Comment #15
JonBob CreditAttribution: JonBob commentedDid I make a suggestion? I'd be okay with either changing hook_help() to return arrays, or changing module_invoke_all() to convert return values into arrays.
Comment #16
Steven CreditAttribution: Steven commentedI was referring to your 'gut feeling':
"My gut feeling is that it would be better to do this directly inside module_invoke_all() rather than in an array_merge() wrapper though, because it would force us to really examine each case of array_merge() and come up with the best solution rather than adding unnecessary checks everywhere."
;).
Comment #17
killes@www.drop.org CreditAttribution: killes@www.drop.org commentedAnother tiny patch.
Comment #18
(not verified) CreditAttribution: commentedThese patches are for CVS version? IS 4.42 going to be patched?
Comment #19
killes@www.drop.org CreditAttribution: killes@www.drop.org commentedYes, the patches are for cvs. No, I don't think that 4.4.2 will be patched as the release of Drupal 4.5 will be in a few weeks.
Comment #20
markc CreditAttribution: markc commentedApologies for a dumb beginners question here but it seems the main developers list is rather hostile towards anything to do with PHP5 mods. I downloaded a recent CVS snapshot tarball and there is no sign of any of the patches on this page. Exactly where are these patches being applied ?
Comment #21
JonBob CreditAttribution: JonBob commentedThey have not been applied, which is why this issue is still marked "patch" and not "fixed."
Comment #22
Dries CreditAttribution: Dries commentedIt would be great if someone could (i) update and (ii) combine these patches in a single patch.
Comment #23
markc CreditAttribution: markc commented> It would be great if someone could (i) update and
> (ii) combine these patches in a single patch.
Here is an attempt at such a unified patch. It's probably all backwards compatible except for the "var" to "public" transform in xtemplate.inc, so it's not suitable to be applied to HEAD. I'm not sure what the PHP5 policy is as there are two issues, a) making drupal forwardly compatible with PHP5 so that, ideally, it passes running under E_ALL | E_STRICT and, b) having a separate branch specifically to take advantage of PHP5 semantics, especially exceptions.
Comment #24
killes@www.drop.org CreditAttribution: killes@www.drop.org commentedThanks a lot! I love it when other people update my patches. :)
There are still two controversial issues about this patch:
1) the xtemplate.inc will only work with php5 now. I think we need a if statement checking for the php version.
2) this part of the patch will break the help hook (and maybe others):
There are two possible solutions:
a) Ensure that all hooks return arrays. I have a script for the help hook which inserts a lot of array declarations.
or
b) check for the return type and act accordingly:
Comment #25
LacKac CreditAttribution: LacKac commentedA new patch for includes/common.inc. This one corrects the error_handler function to be able to handle the new STRICT type of errors.
Comment #26
killes@www.drop.org CreditAttribution: killes@www.drop.org commentedI've merged the last two patches and addressed the module_invoke_all problem as indicated in b).
This patch does not include the xtemplate stuff. Coul dsome theme person have a look at this? I guess we need two xtemplate engines or so.
Comment #27
killes@www.drop.org CreditAttribution: killes@www.drop.org commentedComment #28
TDobes CreditAttribution: TDobes commentedI'm noticing a small problem with the latest patch in PHP 4.3.7 on the "default workflow" page. (and possibly elsewhere as well) Empty items are being added to the array returned by node_invoke_nodeapi, so I have changed the else to an elseif (isset(...)) in the attached patch.
I looked through the rest of the patch and doesn't look like any of the other changes need a fix of this nature. Can someone running PHP5 double-check this and make sure things still work after applying this patch?
Comment #29
Steven CreditAttribution: Steven commentedApplied to CVS.
Comment #30
moshe weitzman CreditAttribution: moshe weitzman commentedAs far as xtemplate goes, I think the best solution is to move the xtemplate engine out of core and reimplement that bluemarine under chameleon or the upcoming phptemplate engine. Xtemplate is indeed braindead, and merits no forther attention from core developers IMO.
Comment #31
killes@www.drop.org CreditAttribution: killes@www.drop.org commentedAs long as we can provide something that resembles an update patch for people who have made minor changes to xtemplate, I'd suport this. Not that people had problems when we removed Marvin and Unconed. We should do better this time.
Comment #32
Steven CreditAttribution: Steven commentedImplementing Bluemarine and Pushbutton in other template engines is not really a problem. The problem is that many sites out there that are using custom xtemplates. We'd need an automatic convertor for that to work.
Comment #33
LacKac CreditAttribution: LacKac commentedHere is a new patch which solves an issue in node.module in a php4 compatible way.
Comment #34
killes@www.drop.org CreditAttribution: killes@www.drop.org commentedI don't really like that patch. I think we should rather use an array in this case. Nevertheless I set it to 'patch'.
Comment #35
Steven CreditAttribution: Steven commentedIf we are going to use that eval() hack, we should do it in a cleaner way. I came up with the slightly more elegant solution of using
clone($obj)
and declaringclone()
as a function in PHP4. That way, code that requires clone() is kept clean, while PHP5 still recognizes it asclone ($obj)
.Comment #36
LacKac CreditAttribution: LacKac commentedIt works this way too, but then you should use references in the created clone function, because it doesn't matter in php4, and we do not want unnecessary copying.
Also it should compare the version to 5.0.0dev, because as I remember this issue affects those versions too.
So the code would be:
Comment #37
LacKac CreditAttribution: LacKac commentedWhat if we change every such an usage of objects in Drupal core to arrays. I think it's not so hard to do, at least the part of replacing
$var->prop
to$var['prop']
is just a matter of time (or a clever regexp).To stay compatible with function calls from contrib modules we could use an
object2array($obj)
conversion function.I've made a little test to see whether this creates new problems, and it doesn't:
When passing
$obj
to the change function, it is passed by reference in php5. One would think that after the function call it became an array, but because of the conversion php doesn't use the reference any more but a new variable.If you used the commented line in the change function and commented the
object2array
call, you would see that the$obj->a
property changed to"X"
outside of the function too.If you also think that this could be a good start in migrating to php5 then I could create a patch. I just didn't want to do something useless and waste my time :).
Comment #38
killes@www.drop.org CreditAttribution: killes@www.drop.org commentedHere is another small compatibility patch for taxonomy.module.
Comment #39
Steven CreditAttribution: Steven commentedI applied the taxonomy fix. We still need to think of a good clone() solution.
LacKac: the reason I had clone() explicitly copy in PHP4 is so that we can adopt a policy of always passing objects by reference. This would make PHP4 and PHP5 behave the same.
In any case, such a change is quite big and will probably introduce more bugs. Let's avoid that for Drupal 4.5 for now.
Comment #40
LacKac CreditAttribution: LacKac commentedThere is another problem with changing to arrays. We cannot stay compatible with contribs because many of the core functions return object. But it's still the better way. If we go on with objects in this way it will produce more and more bugs like the node preview bug not just in the core but in contribs too.
We are not using real objects, just object-like arrays so we should get rid of them.
Comment #41
killes@www.drop.org CreditAttribution: killes@www.drop.org commentedAnother small compatibility fix.
Comment #42
killes@www.drop.org CreditAttribution: killes@www.drop.org commentedSorry, wrong patch. Take this one.
Comment #43
killes@www.drop.org CreditAttribution: killes@www.drop.org commentedExtended patch. Still trivial, please apply.
Comment #44
Steven CreditAttribution: Steven commentedApplied to CVS.
Comment #45
killes@www.drop.org CreditAttribution: killes@www.drop.org commentedI just found that the recent changes to the theme system somehow fixed xtemplate.inc for use with php 5. I'd like to learn how.
I am setting this to "active" again because the clone issue isn't resolved.
Comment #46
killes@www.drop.org CreditAttribution: killes@www.drop.org commentedAnother array_merge issue crept in.
Comment #47
Dries CreditAttribution: Dries commentedCommitted to HEAD. Thanks.
Comment #48
killes@www.drop.org CreditAttribution: killes@www.drop.org commentedPlease revert the last patch. It tried to fix something that doesn't need to and thus breaks a lot of stuff. I only saw the first part of line 346 and didn't see that $arguments was already an array. The culprit was an old module without menu caching being around.
Comment #49
killes@www.drop.org CreditAttribution: killes@www.drop.org commentedReverted, thanks to Steven.
Comment #50
TDobes CreditAttribution: TDobes commentedTwo duplicates have cropped up for xtemplate's incompatibility with E_STRICT.
When I turn on display of all error messages on my PHP5 test install, I seem to get a ton of "var is deprecated" warnings from xmlrpc.inc too... maybe we need to fix them in the same way?
Comment #51
Steven CreditAttribution: Steven commentedThe problems with 'var' are the result of syntax differences between PHP4 and PHP5. I don't think we can get around them without providing two versions of the class definition (either in separate files, or with some conditional code magic, if possible).
Comment #52
killes@www.drop.org CreditAttribution: killes@www.drop.org commentedAnother small compatibility issue, see http://drupal.org/node/12621
Comment #53
Dries CreditAttribution: Dries commentedCommitted the file.inc patch to HEAD and DRUPAL-4-5.
Comment #54
Dries CreditAttribution: Dries commentedSetting to 'active' until a new patch gets attached.
Comment #55
Steven CreditAttribution: Steven commentedI committed a fix for the node previewing/form issue.
Now the only other problem I can think of is the xtemplate "deprecated var" warnings.
Comment #56
gamaralf CreditAttribution: gamaralf commentedThere is a new version for XTemplate, published on April 11, 2005.
Comment #57
killes@www.drop.org CreditAttribution: killes@www.drop.org commentedxtemplate issue was "put under the carpet", xtempalte is removed from HEAD, markingi fixed.
Comment #58
(not verified) CreditAttribution: commentedComment #59
(not verified) CreditAttribution: commentedComment #60
(not verified) CreditAttribution: commentedComment #61
(not verified) CreditAttribution: commented