Problem/Motivation

In core/modules/search/tests/src/Functional/SearchBlockTest.php, $email variable never used.

The variable was never used when this code was first committed so it can simply be removed.

CommentFileSizeAuthor
#11 3173595-11.patch650 bytesPooja Ganjage
#6 removed-unused-variable-3173595-6.patch2.02 KBmeena.bisht
#6 Screenshot 2020-10-08 at 11.56.05 AM.png267.99 KBmeena.bisht
#2 email.png518.32 KBshetpooja04
#2 3173595-2.patch653 bytesshetpooja04
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

shetpooja04 created an issue. See original summary.

shetpooja04’s picture

shetpooja04 CreditAttribution: shetpooja04 at QED42 commented
Assigned: shetpooja04 » Unassigned
Status: Active » Needs review
FileSize
3173595-2.patch653 bytes
email.png518.32 KB

Commit ID: ce189a3f

Link: https://git.drupalcode.org/project/drupal/-/commit/ce189a3f752e15bdeb92f620cff7a1fc72b8de41

File: core/modules/search/tests/src/Functional/SearchBlockTest.php Line: 130

For Issue: https://www.drupal.org/project/drupal/issues/3067943 the variable is added but not used

Please review

ranjith_kumar_k_u’s picture

ranjith_kumar_k_u CreditAttribution: ranjith_kumar_k_u at Zyxware Technologies commented

The above patch applied successfully ,it removes unused variable $email in SearchBlockTest.php.RTBC

ranjith_kumar_k_u’s picture

ranjith_kumar_k_u CreditAttribution: ranjith_kumar_k_u at Zyxware Technologies commented
Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 2: 3173595-2.patch, failed testing. View results

meena.bisht’s picture

meena.bisht CreditAttribution: meena.bisht at QED42 for Drupal India Association commented
Assigned: Unassigned » meena.bisht
Issue summary: View changes
FileSize
Screenshot 2020-10-08 at 11.56.05 AM.png267.99 KB
removed-unused-variable-3173595-6.patch2.02 KB

Adding patch Removing $email variable and doing additional changes as was came across while running phpcs attaching screenshot.

meena.bisht’s picture

meena.bisht CreditAttribution: meena.bisht at QED42 for Drupal India Association commented
Status: Needs work » Needs review
sulfikar_s’s picture

sulfikar_s CreditAttribution: sulfikar_s at Zyxware Technologies commented

Hi,

I've applied the patch on the #6 and it applied cleanly. The patch removes $email variable and also the drupal standards are fixed in the line 96 & 116 (of SearchBlockTest.php) as the array contents are properly splitted over mulitple lines.

RTBC.

sulfikar_s’s picture

sulfikar_s CreditAttribution: sulfikar_s at Zyxware Technologies commented
Status: Needs review » Reviewed & tested by the community
quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs issue summary update

@sulfikar_s, Welcome to Drupal! Nice clear description of what you did to review this issue.

+++ b/core/modules/search/tests/src/Functional/SearchBlockTest.php
@@ -88,12 +88,18 @@ public function testSearchFormBlock() {
+    /** @var \Drupal\search\SearchPageRepositoryInterface $search_page_repository */
...
+      Url::fromRoute('search.view_' . $entity_id, [],

@@ -107,7 +113,13 @@ public function testSearchFormBlock() {
-      Url::fromRoute('search.view_' . $entity_id, [], ['query' => ['keys' => ''], 'absolute' => TRUE])->toString(),

All these changes are out of scope. The issue title and summary states that this issue is to remove an unused variable. Anything outside of that must be done in a separate issue. And coding standard fixes are usually done in bulk by type not on individual files.

And lets update the issue summary. Adding the information that is in #2 would save the reviewer from having to read the issue to find that information. And the screenshot isn't necessary, we all have editors and can look at the file.

Pooja Ganjage’s picture

Pooja Ganjage CreditAttribution: Pooja Ganjage at Asentech LLC commented
FileSize
3173595-11.patch650 bytes

Hi,

I am updating patch as per #10 comments.

Please review the patch.

Thanks.

Pooja Ganjage’s picture

Pooja Ganjage CreditAttribution: Pooja Ganjage at Asentech LLC commented
Status: Needs work » Needs review
Pooja Ganjage’s picture

Pooja Ganjage CreditAttribution: Pooja Ganjage at Asentech LLC commented
Title: Unused variable $email in SearchBlockTest.php, search module » Remove only Unused variable $email in SearchBlockTest.php, search module

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Abhijith S’s picture

Abhijith S CreditAttribution: Abhijith S as a volunteer and at Zyxware Technologies commented

Applied patch #11 on 9.2.x.It works fine

Checking patch core/modules/search/tests/src/Functional/SearchBlockTest.php...
Applied patch core/modules/search/tests/src/Functional/SearchBlockTest.php cleanly.

RTBC

jijojoseph_zyxware’s picture

jijojoseph_zyxware CreditAttribution: jijojoseph_zyxware as a volunteer and at Zyxware Technologies commented
Assigned: meena.bisht » Unassigned
Status: Needs review » Reviewed & tested by the community

Thanks for researching this, I agree with your findings that we don't need this variable.

Patch#11 applied cleanly.

RTBC!!!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 11: 3173595-11.patch, failed testing. View results

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Issue summary: View changes
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs issue summary update

Random fail. Also updated IS.

  • catch committed a33344f on 9.2.x 
    Issue #3173595 by shetpooja04, meena.bisht, Pooja Ganjage, quietone,...

  • catch committed b14d784 on 9.1.x 
    Issue #3173595 by shetpooja04, meena.bisht, Pooja Ganjage, quietone,...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Version: 9.2.x-dev » 9.1.x-dev
Status: Reviewed & tested by the community » Fixed

Committed a33344f and pushed to 9.2.x. Thanks!

Status: Fixed » Closed (fixed)

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