Remove unused local variable and add @return type for
_search_find_match_with_simplify() of 'search.module'.

RC note

Should be rc eligible because it is just removing one line of cruft (this local variable is not referred to anywhere else in search.module after it is set in this line) and adding @return type to API docs.

CommentFileSizeAuthor
#13 2602464-10.patch1.08 KBrakesh.gectcr
#9 2602464-9.patch1.08 KBanil280988
#6 2602464-5.patch1.08 KBheykarthikwithu
#2 2602464.patch1.08 KBheykarthikwithu
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

heykarthikwithu created an issue. See original summary.

heykarthikwithu’s picture

heykarthikwithu
Primary language English
Location Bengaluru 🌍
CreditAttribution: heykarthikwithu as a volunteer and at Valuebound commented
FileSize
2602464.patch1.08 KB
heykarthikwithu’s picture

heykarthikwithu
Primary language English
Location Bengaluru 🌍
CreditAttribution: heykarthikwithu as a volunteer and at Valuebound commented
Status: Active » Needs review
jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Title: Remove unused local variable and PHPDoc comment does not match function _search_find_match_with_simplify() of 'search.module'. » Remove unused local variable and add return value to docs in _search_find_match_with_simplify() of 'search.module'.
Status: Needs review » Reviewed & tested by the community

This is correct. That local variable is indeed never used, and the return docs are correct. Should be rc eligible because it is just removing one line of cruft and adding API docs.

Fixing issue title though, since the patch is just adding the missing return type to the function docs, not fixing some incorrect docs.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 2: 2602464.patch, failed testing.

heykarthikwithu’s picture

heykarthikwithu
Primary language English
Location Bengaluru 🌍
CreditAttribution: heykarthikwithu as a volunteer and at Valuebound commented
Status: Needs work » Needs review
FileSize
2602464-5.patch1.08 KB
heykarthikwithu’s picture

heykarthikwithu
Primary language English
Location Bengaluru 🌍
CreditAttribution: heykarthikwithu as a volunteer and at Valuebound commented
Status: Needs review » Reviewed & tested by the community

Moving back to RTBC, since status was updated by BOT.

jhodgdon’s picture

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

Patch #5 is not quite right. It is missing the |null from the other patch in the return line... and the other patch I just noticed should be |null not |NULL in the @return line.

anil280988’s picture

anil280988 CreditAttribution: anil280988 at Publicis Sapient for Publicis Sapient commented
Status: Needs work » Needs review
FileSize
2602464-9.patch1.08 KB

Changed NULL to null

jhodgdon’s picture

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

Thanks! All correct now. Adding RC note to issue summary.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 9: 2602464-9.patch, failed testing.

anil280988’s picture

anil280988 CreditAttribution: anil280988 at Publicis Sapient for Publicis Sapient commented

On of the Test is aborted, its happening with other patches also. Could you suggest how to handle this.

rakesh.gectcr’s picture

rakesh.gectcr
he/him
Primary language Malayalam
Location Manchester
CreditAttribution: rakesh.gectcr as a volunteer and at Valuebound commented
Status: Needs work » Needs review
FileSize
2602464-10.patch1.08 KB

Recreated the new patch with exactly same changes.

rakesh.gectcr’s picture

rakesh.gectcr
he/him
Primary language Malayalam
Location Manchester
CreditAttribution: rakesh.gectcr as a volunteer and at Valuebound commented
Status: Needs review » Reviewed & tested by the community

The test is passed now, Keeping back to RTBC

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

Thanks! Hm. there used to be a "retest" button.... I guess that doesn't exist in the current test environment.

Anyway you probably could have just set the status back to RTBC and the test would have eventually been re-run. Next time. :)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 13: 2602464-10.patch, failed testing.

jhodgdon’s picture

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

Someone keeps aborting tests on the bot and that is causing issues to get set to Needs Work. We need to fix that. Anyway this is still RTBC.

The last submitted patch, 9: 2602464-9.patch, failed testing.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks!

  • catch committed 81ffdd4 on 8.0.x 
    Issue #2602464 by heykarthikwithu, anil280988, rakesh.gectcr: Remove...

Status: Fixed » Closed (fixed)

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