Closed (fixed)
Project:
Drupal core
Version:
6.x-dev
Component:
documentation
Priority:
Normal
Category:
Bug report
Assigned:
Unassigned
Reporter:
Created:
27 Jun 2009 at 12:48 UTC
Updated:
15 Mar 2010 at 17:10 UTC
locale_form_alter suddenly takes form_state as reference (like &$form_state instead of just $form_state), this change was made with #287178: Use hook_form_FORM_ID_alter() where possible but it doesn't say why and the function also doesn't change anything about form_state, so I'm wondering if it's just a typo that got trough somehow
same goes for translation_form_node_type_form_alter, translation_form_alter, _openid_user_login_form_alter, _color_theme_select_form_alter, and these seem to be pretty much the only functions that do this
Comments
Comment #1
seutje commentedsame for color_form_system_themes_alter, color_form_system_theme_select_form_alter, color_form_system_theme_settings_alter (and the helper functions they call)
Comment #2
berdirThe documentation is by reference too, atleast hook_form_FORM_ID_alter, so it is correct, I think: http://api.drupal.org/api/function/hook_form_FORM_ID_alter/7
Also, it is explicitly called by reference, see http://api.drupal.org/api/function/drupal_prepare_form/7.
What needs to be fixed is the documentation of hook_form_alter: http://api.drupal.org/api/function/hook_form_alter/7
Comment #3
seutje commentedso that would also mean that some hook_form_alter functions need to be changed, as they currently don't take it in by reference
Comment #4
berdirFor consistency, it would probably be better, yes. However, they don't need to, not even with PHP 5.3 ;)
Comment #5
sun$form_state is always passed by reference and all form_alter implementations are free to alter the form's state if required.
I didn't understand that change regarding PHP 5.3 in full detail yet. $form_state is an array, so at least for PHP 5.2 and below it needs to be passed by reference to make it alterable.
Comment #6
seutje commentedhm, I see
could there be any performance gain if we didn't pass it by reference when it's not needed? should we change all similar functions to be consistent, or should we not do this when it isn't necessary?
Comment #7
effulgentsia commentedDocumentation in http://api.drupal.org/api/function/hook_form_alter/7 looks ok to me. drupal_alter() has been refactored to be performant, work in PHP 5.2 and PHP 5.3, and take all of its parameters by reference. Each hook_*_alter() implementation takes its first parameter by reference, and may choose whether to take its optional 2nd and 3rd parameters by reference or not.
Comment #8
berdirCan we document this correctly for D6?
The & for $form_state is missing for hook_form_alter() and has lead to a lot of confusion due to PHP 5.3 related bugs, there are many reports stating that it needs to be removed but the real issue is that some modules call the hook wrong, for example comment_upload and cck. See http://api.drupal.org/api/function/hook_form_alter
Comment #9
sunAgreed.
Comment #10
jhodgdonI'm not understanding what documentation change needs to be made for Drupal 6....
Let's see. Currently, hook_form_FORM_ID_alter() is saying $form_state (not by reference), but hook_form_FORM_ID_alter() is saying &$form_state (by reference) in D6 doc.
They are both invoked in drupal_prepare_form() by calling drupal_alter(), and in both cases, the $form_state variable is made available to drupal_alter() to pass by reference.
So it looks like hook_form_alter() should have &$form_state -- is that what everyone is advocating changing in the doc? That's all I can see that might be wrong.
---
That aside, I do not think it is a problem that some form_alter functions decided not to declare $form_state a parameter by reference. If an individual function is not modifying $form_state, then there is no reason for that function to declare $form_state a parameter by reference.
Comment #11
sunI've fixed the hook_form_alter() documentation for 6.x.