API page: http://api.drupal.org/api/drupal/core%21modules%21search%21search.pages....

The code executed from the function (at the end of the function) is the following one.

  $form_state['redirect'] = $form_state['action'] . '/' . $keys;
  return;

The return statement is useless, as the function is already returning to the caller.

CommentFileSizeAuthor
#8 d6-remove-return-statement-from-search_form_submit-function-1817484-8.patch418 bytestomgeekery
#2 remove-return-statement-from-search_form_submit-function-1817484-2.patch363 bytestomgeekery
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Issue tags: +Novice
tomgeekery’s picture

tomgeekery CreditAttribution: tomgeekery commented
Assigned: Unassigned » tomgeekery
Status: Active » Needs review
FileSize
remove-return-statement-from-search_form_submit-function-1817484-2.patch363 bytes

I am attaching a patch that removes the return statement from the search_form_submit function in search.pages.inc. Should this be back ported to D7 as well?

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs review » Reviewed & tested by the community

Makes sense to clean this up, if this code exists in 7.x too, then it could be backported, not *that* important, though.

apaderno’s picture

apaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
CreditAttribution: apaderno commented

The code of that function has not been changed since Drupal 6. On Drupal 5, the code was the following one.

  $keys = $form_values['processed_keys'];
  if ($keys == '') {
    form_set_error('keys', t('Please enter some keywords.'));
    // Fall through to the drupal_goto() call.
  }

  $type = $form_values['module'] ? $form_values['module'] : 'node';
  return 'search/' . $type . '/' . $keys;

In Drupal 6, the function uses $form_state['redirect'] = 'search/' . $type . '/' . $keys; but leaves the return statement which doesn't return anything.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Title: The function contains a "return" statement right at the end of the function » The search_form_submit() function contains a "return" statement right at the end of the function
Version: 8.x-dev » 6.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed and pushed to 8.x and 7.x. Thanks!

Not sure about backporting such clean-up patches all the way to 6.x, but I'll move it there for porting.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Issue tags: +Needs backport to D6
apaderno’s picture

apaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
CreditAttribution: apaderno commented

I am not sure there is the need of changing the code for Drupal 6; it is not a bug, and leaving that statement doesn't have any bad consequences, especially because it's the last statement in the function.

tomgeekery’s picture

tomgeekery CreditAttribution: tomgeekery commented
Status: Patch (to be ported) » Needs review
FileSize
d6-remove-return-statement-from-search_form_submit-function-1817484-8.patch418 bytes

I am attaching a patch for D6. I am still pretty new at writing patches so thank you for the practice!

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs review » Closed (won't fix)

Thanks for the backport! At this point I do not think we are committing anything except vital bug fixes to Drupal 6... and we had a gap where no one was maintaining the search module so your patch didn't get noticed back when it might have gotten committed. Sorry about that!

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Closed (won't fix) » Reviewed & tested by the community

Actually, I was wrong. there is a chance this could be committed, as it's an obvious cleanup and will definitely not cause any problems to commit it. So I'll mark it as RTBC and we can see what happens.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 8: d6-remove-return-statement-from-search_form_submit-function-1817484-8.patch, failed testing.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Issue summary: View changes
Status: Needs work » Reviewed & tested by the community

test bot glitch!

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

8: d6-remove-return-statement-from-search_form_submit-function-1817484-8.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 8: d6-remove-return-statement-from-search_form_submit-function-1817484-8.patch, failed testing.

Status: Needs work » Needs review

jhodgdon queued 8: d6-remove-return-statement-from-search_form_submit-function-1817484-8.patch for re-testing.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 8: d6-remove-return-statement-from-search_form_submit-function-1817484-8.patch, failed testing.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs work » Reviewed & tested by the community

These errors are unrelated to the patch, but I'm not going to click "retest" any more...

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Version: 6.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Fixed

Talked with Gabor (the Drupal 6 branch maintainer) and D6 issues are really not being committed unless they're really essential -- we really don't have a test system for Drupal 6 and it's too dangerous. So... putting this back to D7 / fixed, even though this is a simple one-line patch.

Status: Fixed » Closed (fixed)

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