Problem/Motivation

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

Proposed resolution

Remove it

CommentFileSizeAuthor
#6 2778655-6.patch489 bytesjoelpittet
#2 remove_unused-2778655-2.patch469 bytesjoelpittet

Comments

joelpittet created an issue. See original summary.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Active » Needs review
StatusFileSize
new remove_unused-2778655-2.patch469 bytes
megachriz’s picture

megachriz
Primary language Dutch
commented

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

joelpittet
he/him
Primary language English
Location Vancouver
commented

@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

twistor commented
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

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Needs work » Needs review
StatusFileSize
new 2778655-6.patch489 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

megachriz
Primary language Dutch
commented
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.