PHP 4 parse error in filefield_meta

I get the same when installing the module.

That's a PHP 4 error - I believe PHP 4 cannot handle default values for reference parameters. I'm also not sure if it's a good idea to have a default value when two parameters without default value follow - kinda makes the concept of a default value unworkable in this case. Don't know why this line changed that way, though... dopry?

Appendix A. Here's the concerned line:

function filefield_field($op, $node, $field, &$items = array(), $teaser, $page) {

Likewise. My test site is now unusable ...

cuz I don't use php4, and don't notice those bugs... You can probably remove it if it doesn't break anything.

Ok, fixed in latest CVS (DRUPAL-5--2 branch). Please test that CVS version or the next release candidate when it comes out, and make sure the error is gone for good - I don't use PHP 4 too, so there might be problems remaining in some other places.

Fixes the first error, but now I'm getting:

Parse error: syntax error, unexpected '=', expecting ')' in /home/site/public_html/sites/all/modules/filefield/filefield_meta/filefield_meta.module on line 25

When enabling the filefield_meta module. Same problem?

Here is how I've made it work under PHP 4.

Change in filefield/filefield_meta.module (no default value for param passed by reference):

function filefield_meta_filefield($op, &$node, $field, &$file, &$form) {
// was: function filefield_meta_filefield($op, &$node, $field, &$file, &$form = NULL) {

Change in filefield.module (dummy $form array passed as missing parameter):

    // let modules massage act on the file.
    foreach(module_implements('filefield') as $module) {
      $function =  $module .'_filefield';
      $dummyform = array();
      $function('file_prepare', $node, $field, $file, $dummyform);
    }

and similar changes for file_(save|delete|load).

Is there a reason the form is passed by reference?

My fixed worked in a similar manner to what is done in #8. The difference is I passed NULL instead of array as the form. So, it looked this.

// let modules massage act on the file.
    foreach(module_implements('filefield') as $module) {
      $function =  $module .'_filefield';
      $function('file_prepare', $node, $field, $file, NULL);
    }

NULL was the default setting in filefield_meta_filefield. Though, it may be better to pass an empty array instead of NULL. Any thoughts so we can roll a patch?

Well... the point of "$form = NULL" is that the form won't be passed to the hook in all cases, and only makes sense for $op = 'file_form'. So, while passing this parameter in all cases would make the module work under PHP 4, it wouldn't be the cleanest of all solutions. I vote for doing filefield_meta (and other filefield extensions, too) as PHP-5-only module. It's a new module after all so there's no regressions, PHP 4 is unsupported already, and y'all should really switch to PHP 5.

Apart from that, I *could* imagine one possible solution in leaving out the $form argument in the hook_filefield() implementation altogether, and retrieving it with func_get_args(). Not that I like this very much, but it's the most viable of all solutions that can make this work in PHP 4.

dopry? wontfix or func_get_args()?

Now that I think of it, func_get_args() only returns a copy of the passed argument only, so we can't use this as a reference as would be needed. That makes it easy to decide for the wontfix resolution. Sorry dudes... at least filefield itself works with PHP 4 now.

Can we get this documented somewhere? I'll live with this being php 5 only as long as we can get this documented so others don't go crazy trying to figure out why this doesn't with with php 4.

better solution: split filefield_meta_filefield in two variants, one that has the form parameter and one that hasn't. I think there is only one call where that parameter is used, in most instances it's not. Better to have two functions. This is an easy win, remaining compatible with PHP 4. Not everybody gets to choose which PHP version their hosting company enables. And as a design rule a default parameter value should be meaningful, not a trick to merge two functions into one. Use a class if you need overloading.

In my case, I'm going to try to get my client's hosting company to switch to PHP 5 -- but I still think from a design POV it's not good to merge two functions into one with the trick of making a (pass-by-reference) parameter equal to NULL.

I made this tiny patch (not needing review) passing it by variable instead of reference.

@magnusk: I disagree with the argument that splitting the function should be done in order to remain compatible with PHP 4 - imho, new functionality should not need to be compatible with unsupported legacy versions. Also, even if you can't choose your hosting company's technology, you can choose your hosting company itself, and there's lots and lots of hosts that provide PHP 5 for the same amount of money.

I do find the design argument much more valuable, though. Personally, I agree that different stuff should not be handled by the same function, and would appreciate a patch showing how to do it that way. Anyways, as hook_filefield() (and hook_file(), which is strongly related to the former) is dopry's work, it'll be his decision whether or not to take your proposal to heart.

@DynV: Unfortunately, it's being reviewed nevertheless, and
1. the patch does the opposite of what you intended - you posted a reverse patch.
2. we want that parameter to be passed by reference, because that's how CCK fields are supposed to do it - see the CCK docs. And finally,
3. filefield_field() is already fixed in CVS. We're now talking about filefield_meta's implementation of hook_filefield(), hence the new issue title.

There's no such thing as a patch not needing review, even if it just affects one single character.

jpetso

There's no such thing as a patch not needing review, even if it just affects one single character.

I beg to differ ! The review process I was addressing was to verify that its claims are actually done and I would be really surprised is anyone would doubt that items_variable.patch didn't do passing it by variable instead of reference. Just look at the section Verify your patch from Submitting patches.

@DynV: Yes, it works if our goal is to not pass it by reference. Nevertheless, that's not what is intended here, so while the patch is ok from a technical point of view (apart from being a reverse patch), it doesn't fix the problem at its core or provide input *why* we want to use this specific solution. To consider these questions is the responsibility of the module maintainer(s), and in consequence, those need to do a review as well, still.

Well, anyways... still waiting for input from dopry.

jpetso thanks for the patch reversal warning, I fixed it.

That's what I meant, the patch might not need a review (patch bingo) however, to be included in a release its specification need to be reviewed which could be done by a non-technical project manager.

Sorry PHP4 folk... go read goPHP5.org and talk to your ISP about it... I'm not gonna do the backwards compatibility thing.. I did however add a hook_requirements implementation to filefield and filefield_meta

meant won't fix.

Well, mmkay. However, the hook_requirements() check is relatively pointless: if PHP 4 people enable the module, they won't get to see it in the first place - they'll rather see the parse error first :-o

I updated the project page with the new requirement, probably the README.txt should include a hint on this as well.

Well my host is not planning to upgrade to PHP 5 before 3 months so this is a major drawback ! There's no other way, even with a complicated workaround ?

DynV,

filefield-5.x-2.2.tar seems to work in PHP4.
Nodes:

167207
180479
177638

contain bug fixes, but I don't know which of these are PHP5 specific. You can see more bug issues/fixes details here: http://drupal.org/node/77863/release