backup_migrate PHP 5.3 compatibility

I had the same problem and this cured it:

Looks like a PHP 5.3 issue. Thanks for the patch. Applied to the latest dev.

Glad I could help. Have a nice day :) .

But now PHP 5.2 complains that "Call-time pass-by-reference has been deprecated".
Any suggestions?

Damn, I was worried about that. If nobody has any ideas on how to fix this then I'll probably go back to the 5.2 compatible version since that's what's supported by Drupal 6 core.

Apparently if an empty array is passed by reference through call_user_func_array, it actually passed NULL instead. That's a new "feature" of PHP5.3.
In many cases during backup_migrate_filters_invoke_all, call_user_func_array is being called with an empty $args. A hacky fix would be to force $args to be non-empty.
However, it looks like someone was able to fix this by simply casting $args as an array.

Can anyone test this patch on PHP5.3?

If that works then we're in business. Any PHP 5.3 users able to verify this patch? @Chipzzz?

While we're waiting for someone with PHP 5.3 to test this, who here knew that you are supposed to include whitespace between the cast-type and the variable when casting? I didn't.
Rerolled with the whitespace added.

Sorry to spam, but that was a really vague title. Maybe now people will feel compelled to review this.

Sorry to keep you waiting. I'm pleased to report that patch #8 works perfectly with PHP 5.3 :)). Nice work, tim!

awesome!

I finally tested this myself on php5.3, and I can also confirm that it works. Still RTBC.

People. No.

The patch in #8 is exactly what the code was before this issue was opened. So this cannot solve the issue.
(The only difference in the code is that it casts the $args array ... to an array. Which doesn't make much of a difference.)

That was theory. My practice -testing the patch in #8 with PHP5.3- indeed reveals the original error message again.

Here's another patch. With comments. It should work on both 5.2 and 5.3, but I only tested it on 5.3.

If you were to read my comment in #6, and the accompanying PHP documentation, you'd see that casting it as an array is EXACTLY the solution.
I see no error message with the patch at #8 applied in PHP 5.3 or PHP 5.2

Also, the whole reason this issue is open is because the current code DOES work with PHP 5.3 and DOES NOT work with PHP 5.2, so writing a patch and not testing it on PHP 5.2 is pointless.

Also, for some minor nitpicking of your patch, read up on switch statements.

If you were to read my comment in #6, and the accompanying PHP documentation, you'd see that casting it as an array is EXACTLY the solution.

...in the case where $args is empty.

However, $args is (of course) not always empty. It's definitely not empty in the situation that caused the issue to be opened (a form_validate warning). And nothing changed from the original situation. You just restored the behaviour to the situation where spacereactor reported the issue. So how would that _solve_ the issue?

What's more: the code needs to pass $args[1] by reference to e.g. the form_validate function. Failure to do that, will mean deviation from specifications, meaning weird bugs for some people who implemented these form_validate functions.

I see no error message with the patch at #8 applied in PHP 5.3 or PHP 5.2

I do, as I said. (Which makes me suspect you didn't test properly, but I didn't want to be too obvious about that at first.)

Go to YOURSITE/admin/content/backup_migrate/export/advanced, and click 'Backup now'. (Or better, I guess: click "save without backing up".) You have to do (something like) this, in order to get to the form_validate function which was the cause for the original issue report.

Do you not see warning messages on PHP5.3? I do.
If somehow you don't.. maybe you have some more permissive settings in your php.ini than I do, which I do not know about.

In that case, please put some code in the backup_settings_form_validate() function at line 213 of filters.inc, for a test.
- put a breakpoint there. Does that code ever get evaluated? For me it doesn't; it just generates the warning mentioned in the first sentence of the original issue.
- If somehow it does evaluate for you.... change $form_state in that function. Is $args[1] in the caller still changed, after the function returns, as it's supposed to?

Also, the whole reason this issue is open is because the current code DOES work with PHP 5.3 and DOES NOT work with PHP 5.2, so writing a patch and not testing it on PHP 5.2 is pointless.

I do not think the current code (as it is downloadable now, after having patch #1 applied) works as it is supposed to do, at all. I think it introduced a bug.
So I am trying to fix that, even if you say I shouldn't.

You don't even have to test this. Just look at the code. Right now, it passes &$args as the second argument to call_user_func_array().
This backup_settings_form_validate($form, &form_values) is supposed to fill $form with $args[0] and $form_values with $args[1] (by reference). Agreed?
So after applying the patch in #1... what does the code currently do? It fills $form with $args (by reference) and does not fill $form_values. Or am I wrong here?

So I think writing a patch to get rid of the bad behaviour introduced by the patch in #1, is far from pointless. Now if someone could test this (also on PHP5.2) if they want to, that would be very much appreciated.

Also, for some minor nitpicking of your patch, read up on switch statements.

Humour me. I must be having a not very bright day, because I am not catching what you mean here.

I meant that your switch statements didn't have newlines.
I still don't see any errors with my patch applied, but I'm suspecting that it's because of my specific configuration.

I apologize if you felt I was being contentious, but you should probably take it easy with your sweeping "People. No."

Your patch doesn't throw any errors, but I am confused by one thing.
The comment (which is probably too long) implies that call_user_func_array() still expects a reference in certain cases, but there is nothing ever passed by reference.

I attached a patch against CVS with the appropriate spacing issues fixed.

Well sorry for being contentious, but these were only two words. And they did reflect my ... deep surprise at the whole conversation:

- patch in #1 is obviously a bug. (Said shorter than I did before: call_user_func_array() expects an array as the 2nd argument. The patch just changes that 2nd argument into a scalar. I mean... wut?) And it just got committed.

- patch #8 really does fix the error introduced in #1, by reverting to the old situation -- and I have never heard of the 'include white space' issue you mentioned there. (And btw, the old code did not have white space there either.)

So I was having myself a proper 'wtf moment'.

@ronan: I know I'm being a bitch. But I have a 'Node Monkey' sticker with a half cog wheel logo stuck on my laptop cover, if that makes you like me better? :-p

------

OK back to issues:

The comment (which is probably too long) implies that call_user_func_array() still expects a reference in certain cases, but there is nothing ever passed by reference.

I see I'm being ambiguous in my wording. (Maybe the comment isn't long enough? :-p )
Where I said 'the function' twice, I meant the function which is being called (i.e. $filter->$op). These expect a reference in certain cases. They're defined like that. (Which is pretty much what the original warning in this issue is about.)

And that's all fine, because that's all taken care of automatically... except when using call_user_func_array(). With call_user_func_array(), you have to specify references 'at call time'.

So there's only a few sub ideal solutions here, to make things work in both 5.2 and 5.3 without errors:

- change all function definitions so that they don't expect a reference (which is not an option, because then you can't change $form_state anymore in the called function)

- make backup_migrate_filters_invoke_all() always expect a fixed number of arguments, and always expect the 2nd argument by-value and the 3rd argument by-reference (for example... or something like that)

- ditch backup_migrate_filters_invoke_all() completely, because there is no way to make this generic

- make logic like in my patch, which tiptoes around using call_user_func_array() (and which hopes that there are no called functions ($filter->$op) that define more than 2 arguments and define one of those arguments as a reference... because then this patch needs to be extended again.)

Actually, the comment can indeed be shortened without losing any value.

// call_user_func_array() ignores the function signature.
// So we need to work around call_user_func_array() to accomodate
// functions which want references passed as arguments.

(i.e. that's not worded worse than I did in the patch)

Could you reroll a patch with the changed comments?

Style issues aside can someone confirm that the patch in #16 works in both 5.2 and 5.3?

got same error message with php 5.2.8 with latest Dev version
the patch in #16 fixes this

ok. Comments even different than before.

Problem solved with this patch

Thanks GiorgosK!
Your "backup_migrate-785908-22.patch" fixes the problem for me too.

Great, thanks all.