Diff broken with PHP 5.3

Just writing to confirm the bug... We are two at work with the same problem, both on php 5.3.0. The others are fine.

Do you really need to pass the $node as reference in diff_diffs_overview(&$node) ?

It's the same for 2.1-alpha3.

I tried to remove all & in
lines 92, 145, 182, 226 in diff.module
and in lines 7, 22, 116, 126, 145, 234, 319, 339 in diff.pages.inc

but beside the warning disappearing (good) the diffs were not shown still (bad). - So either I removed too much, or something else is broken also.

Yes I have updated several modules including this one to the alpha3 and it was showing me something like this when trying to compare a revision to the current revision:

Fatal error: Call to undefined function diff_diffs_show() in C:\xampp\htdocs\TestWeb\Example1\modules\revisioning\revisioning.module on line 430

I tried the last dev revisioning module and the same behavior, the I come back to alpha2 and it was gone, but when a try to compare two revisions they are not shown. I'm using php 5.3

well I don't sure if this the cause of my problems, because my drupal installation starts to give problems after installing some block related modules. Howewer the php item seems to be true.

revisioning 6.x -2.7
diff 6.x-2.1-alpha2
xampp 1.7.2
drupal 6.14

Worth mentioning that this is a problem in the current stable release too (6.x-2.0). Removing the & in the following function definitions appears to fix the problem:

* diff_diffs_overview();
* diff_node_revisions(); - shouldn't the & be on $form_state instead?
* diff_diffs_show();

Dropping the & in the above function definitions got things working for me. Not sure if this will work for the alpha.

@ #4, That allows the revisions page to appear, but brings the following warnings:

* warning: Parameter 1 to node_diff() expected to be a reference, value given in C:\wamp\www\includes\module.inc on line 471.
* warning: Parameter 1 to taxonomy_diff() expected to be a reference, value given in C:\wamp\www\includes\module.inc on line 471.
* warning: Parameter 1 to content_diff() expected to be a reference, value given in C:\wamp\www\includes\module.inc on line 471.
* warning: date_diff() expects parameter 1 to be DateTime, object given in C:\wamp\www\includes\module.inc on line 471.

Node revision comparisons still appear to be nonfunctional, stating "No visible changes" despite changes being present.

Modified a clean 6.x-2.0 install, PHP 5.3.

I'm experiencing the same issue - Revisions page is blank when you click on it. When you go back a page, you see:


warning: Parameter 1 to diff_diffs_overview() expected to be a reference, value given in ...\menu.inc on line 348.


This wasn't an issue when there were around 10 revisions. My site now has close to 1000 nodes and that's when this started acting up. Has anyone else seen similar effects?

Can confirm the same issue.

I've removed many incorrect reference symbols (&'s). My patch is attached.

tagged

please upload a proper patch.use diff -n

Here's a proper patch in RCS format.

+++ ./diff.module	2010-02-27 11:02:36.505887021 +0530
@@ -142,7 +142,7 @@
 /**
  * Implementation of hook_nodeapi().
  */
-function diff_nodeapi(&$node, $op, $teaser, $page) {
+function diff_nodeapi($node, $op, $teaser, $page) {
   if ($page && $op == 'view' && user_access('view revisions') && variable_get('show_diff_inline_'. $node->type, FALSE)) {
     // Set the hilight flag if specified in the URL
     if (isset($_GET['diff'])) {

This is wrong, the by reference is needed here.

+++ ./diff.pages.inc	2010-02-27 11:04:09.945032843 +0530
@@ -316,7 +316,7 @@
+function _diff_get_next_vid($node_revisions, $vid) {

It's not needed currently to be by reference, but also not necessary to remove.

Note that there are also wrong hook implementations in content.module and date.module and probably more..

Powered by Dreditor.

Fixed. Check the new patch.

Still getting the following error after applying the patch at #13 to 6.x-2.1-alpha3:

warning: Parameter 1 to content_diff() expected to be a reference, value given in /usr/local/drupal/pressflow-6.16.77/includes/module.inc on line 483.

Thanks.

That needs to be fixedi in CCK, not here.

Thanks.

http://drupal.org/node/714762

I was happy to see this ticket. I'm just seeing the ripple effect of PHP 5.3 rollout on my Drupal site. Apparently the two major effects of PHP 5.3 are strict checking of by-value versus by-reference argument passing, and obsolescence of the ereg regular expression library.

#13 works nicely for 6.x-2.1-alpha3

Nevertheless I get the following error message:
warning: date_diff() expects parameter 1 to be DateTime, object given in [myinstallation]/includes/module.inc on line 483.

Also I noticed you are using pass by ref. to go into module_invoke_all() in the routine _diff_body_rows(). You should be aware there is an issue with module_invoke_all() and pass by ref. which has caught many people because it uses the function call_user_func_array() and people don't realize it changes the rules for passing variables, as in:

http://us2.php.net/manual/en/function.call-user-func-array.php
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.

The date_diff warning message (expects parameter 1 to be DateTime, object given in ...) is due to a clash with PHP 5.3's date_diff() function when date.module is enabled. It is not wrong hook implementation (date.module don't implement hook_diff).

The attached patch fixes the problem avoiding use module_invoke_all(), also includes parameters by reference fixes.

Patch based on #20, plus:

• Code clean up with code-clean.sh.
• Coding style cleanup with coder.module + "minor (most)" warning level.

Fully tested with Drupal 6.17 + Debian Squeeze + PHP 5.3.2-1 + diff 6.x-2.x-dev, with date.module DISABLED.

BTW, I would like to suggest not avoiding the use of module_invoke_all() but rename hook_diff() as something other. E.g. we may rename it as hook_ddiff(), which similar as devel.module dprint_r() function.

For sure that may generate more workload for patching, but this would be a long term solution since date_diff() is an official function call for PHP >= 5.3.0 which we shouldn't conflict with.

Patch based on #21, plus:

• Revoke with $node_diffs = module_invoke_all('ddiff', $old_node, $new_node); implementation.
• Rename hook_diff() functions as hook_ddiff().

Fully tested with Drupal 6.17 + Debian Squeeze + PHP 5.3.2-1 + diff 6.x-2.x-dev, with date.module ENABLED.

BTW, if this solution is acceptable, we should also patch cck/includes/content.diff.inc as required.

IMHO, rename hook_diff() must be discussed in a separate issue because it involves a change in the API.

Patch #20 uses module_invoke() for each module enabled, as module_invoke_all(), the only difference to avoid call date_diff() if date.module is enabled.

@smartinm: agree that rename of hook_diff() should belongs to other individual discussion; #21 just target to demonstrate its possibility.

Patch revoke via #20, plus some @todo and @link comments for any other follow up issues if required. No other else changes.

If the patch looks fine, should we RTBC this issue?

[edited post] The date_diff warning is gone. #24 is perfect :)

tracking

I'm not quite sure about the patch in #24, specifically because it mixes a lot of unrelated white spaces fixes and function signature changes with what is a legitimate fix.

Let's focus on the 5.3 fix here, and move the other fixes to other issues.

After taking a close look here I think that the patch in #20 is what we want to commit. Setting this thread to RTBC based on that patch.

Patch in #20 committed here: http://drupal.org/cvs?commit=405564

Agreed that any hook rename conversations can be moved to another thread.

I know this is closed, but does anyone know if this has been pushed through to the D7 version?

It looks like this made it into 7.x-2.x, but not into 7.x-1.x. Is 7.x-1.x even an active branch anymore? That's what drush dled by default...

Sorry! Didn't mean to change issue status.

Hi, can anyone familiar with the issue verify if this has been ported.
Does comment http://drupal.org/node/639320#comment-4230860 verify this?

Thank you,