Problem/Motivation

Noticed the parameter $form_status was incorrectly named and not used.

Proposed resolution

Remove it

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

joelpittet created an issue. See original summary.

joelpittet’s picture

Status: Active » Needs review
FileSize
469 bytes
MegaChriz’s picture

I think that parameter should have been called $form_state instead of $form_status. I think it is not unusual to have that parameter defined on form callbacks even if you do not make use of it?

The coding standards aren't clear about whether or not $form_state is required or should be removed when not used. According to the examples, it seems like it is common to have it always defined.
https://www.drupal.org/coding-standards/docs#forms

joelpittet’s picture

@MegaChriz yes that is correct. And there is no way to modify that unused variable inside that function so defining it is fairly pointless. The $form_state is initialized in drupal_get_form() and at that point only contains $form_state['build_info']['args'] = $args; If you aren't passing it to an alter or modifying it, it's not needed.

twistor’s picture

Status: Needs review » Needs work
+++ b/feeds_ui/feeds_ui.admin.inc
@@ -48,7 +48,7 @@ function feeds_ui_mapping_help() {
-function feeds_ui_overview_form($form, &$form_status) {
+function feeds_ui_overview_form($form) {

Lest's rename the variable to $form_state, add the array type hinting, and call it a day. All form functions take a $form and a $form_state. Whether we use the $form_state isn't important. If there was a performance win here, I would still say no, but there isn't :) The signature for forms is ($form, $form_state). This is even required for all of our class-based forms.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
489 bytes

@twistor, ok it's your call. I just noticed PHP Storm saying it's not used so decided to remove it.

MegaChriz’s picture

Title: Remove unused $form_status feeds_ui_overview_form() » Rename $form_status in feeds_ui_overview_form() to $form_state
Status: Needs review » Fixed

Committed #6. Thanks for spotting the wrong named parameter, joelpittet!

I agree that in most cases it is better to remove unused parameters, but in this case I agree with twistor here. I think the code is more consistent if $form_state is defined, because nearly all form constructors have it even not used.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.