hook_node_view and hook_node_load should not use & in their parameters [#765308]

Object's are passed by reference by default, so there's no need to state that the first parameter to webform_node_view() is a reference.

In fact, doing so displays the following warning:

Warning: Parameter 1 to webform_node_view() expected to be a reference, value given in module_invoke_all() (line 708 of www/includes/module.inc).

Comment	File	Size	Author
#4	hook-parameters-765308-4.patch	983 bytes	JohnAlbin
#4
#1	hook-node-view-765308-1.patch	424 bytes	JohnAlbin
#1

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

File	Size
hook-node-view-765308-1.patch	424 bytes

Comment #2

quicksketch

he/him

CreditAttribution: quicksketch commented 8 April 2010 at 07:35

While true, it sounds like something is being passed into webform_node_view() that cannot be passed by reference at all (such as NULL) which is causing that error. Manually specifying the ampersand may not be necessary, but it shouldn't be causing such a warning.

Comment #3

JohnAlbin

he/they/他

English

Taipei, Taiwan

CreditAttribution: JohnAlbin commented 8 April 2010 at 12:05

Title:	first paramter to webform_node_view() is object, not reference	» hook_node_view and hook_node_load should not use & in their parameters
Priority:	Normal	» Critical
Status:	Needs review	» Needs work

This turns out part of a bigger problem. On a clean CVS install with PHP 5.3.1, when I try to add a component to a webform-enabled node type, I get the following failure:

Warning: Parameter 1 to webform_node_load() expected to be a reference, value given in DrupalDefaultEntityController->attachLoad() (line 255 of includes/entity.inc).

Notice: Undefined property: stdClass::$webform in webform_component_edit_form() (line 409 of sites/all/modules/webform/includes/webform.components.inc).

Notice: Undefined property: stdClass::$webform in webform_component_edit_form() (line 438 of sites/all/modules/webform/includes/webform.components.inc).

Warning: Invalid argument supplied for foreach() in webform_component_edit_form() (line 438 of sites/all/modules/webform/includes/webform.components.inc).

Notice: Undefined property: stdClass::$webform in webform_component_edit_form_validate() (line 548 of sites/all/modules/webform/includes/webform.components.inc).

Warning: Invalid argument supplied for foreach() in webform_component_edit_form_validate() (line 548 of sites/all/modules/webform/includes/webform.components.inc).

Notice: Undefined index: pid in webform_component_insert() (line 658 of sites/all/modules/webform/includes/webform.components.inc).

PDOException: SQLSTATE[23000]: Integrity constraint violation: 1048 Column 'pid' cannot be null: INSERT INTO {webform_component} (nid, cid, pid, form_key, name, type, value, extra, mandatory, weight) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3, :db_insert_placeholder_4, :db_insert_placeholder_5, :db_insert_placeholder_6, :db_insert_placeholder_7, :db_insert_placeholder_8, :db_insert_placeholder_9); Array ( [:db_insert_placeholder_0] => 120 [:db_insert_placeholder_1] => 1 [:db_insert_placeholder_2] => [:db_insert_placeholder_3] => test [:db_insert_placeholder_4] => test [:db_insert_placeholder_5] => textfield [:db_insert_placeholder_6] => [:db_insert_placeholder_7] => a:3:{s:8:"disabled";i:0;s:6:"unique";i:0;s:20:"conditional_operator";s:1:"=";} [:db_insert_placeholder_8] => 0 [:db_insert_placeholder_9] => 0 ) in webform_component_insert() (line 667 of sites/all/modules/webform/includes/webform.components.inc).

And when just viewing a node of that type I get:

Warning: Parameter 1 to webform_node_load() expected to be a reference, value given in DrupalDefaultEntityController->attachLoad() (line 255 of includes/entity.inc).
Warning: Parameter 1 to webform_node_view() expected to be a reference, value given in module_invoke_all() (line 708 of includes/module.inc).

Ah, crap. I just discovered its a bug in PHP 5.3.1. http://bugs.php.net/bug.php?id=50394 That explains the "expected to be a reference, value given" part of the warning.

I still think webform shouldn't be using & in the params for those two hooks, but I can't really test a fix on this broken PHP 5.3.1 system. (which, btw, is the default PHP on the Mac OS X 10.6.3.)

Comment #4

JohnAlbin

he/they/他

English

Taipei, Taiwan

CreditAttribution: JohnAlbin commented 8 April 2010 at 12:10

Priority:

Critical

» Normal

File	Size
hook-parameters-765308-4.patch	983 bytes

Whoops. The PHP bug is critical. Can't tell the severity of the webform bug.

Here's a patch.

Comment #5

JohnAlbin

he/they/他

English

Taipei, Taiwan

CreditAttribution: JohnAlbin commented 14 May 2010 at 18:39

Status:

Needs work

» Needs review

Needs review!

Comment #6

quicksketch

he/him

CreditAttribution: quicksketch commented 15 May 2010 at 05:20

I still can't telly why this is necessary or reproduce any problems with the current approach. Could you provide any way to reproduce an error?

Comment #7

Stevel CreditAttribution: Stevel commented 30 May 2010 at 12:28

I've reproduced this bug with 7.x-3.0-beta5, and drupal 7 beta 5. The PHP-version is 5.3.2, the default version in the latest Ubuntu LTS version. I got the same as in #3. The patch provided in #4 solves the problems as far as I can tell.

I also agree that apart from this php error, the two mentioned hooks shouldn't use the & as per http://api.drupal.org/api/function/hook_node_load/7 and http://api.drupal.org/api/function/hook_node_view/7

Comment #8

Stevel CreditAttribution: Stevel commented 30 May 2010 at 13:51

Status:

Needs review

» Reviewed & tested by the community

Comment #9

quicksketch

he/him

CreditAttribution: quicksketch commented 18 June 2010 at 01:51

Category:	bug	» task
Status:	Reviewed & tested by the community	» Fixed

Ah well, even though I'd argue these changes are 100% unnecessary and indicative of a larger problem with either the implementation around Webform or (in this case apparently) directly with PHP. The & shouldn't make any difference at all.

But as is these lines are changed anyway between the D6 and D7 versions so I can't really make a claim for consistency between branches. Committed to D7 branch.