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

Comments

shetpooja04 created an issue. See original summary.

shetpooja04’s picture

shetpooja04 commented
Assigned: shetpooja04 » Unassigned
Status: Active » Needs review
StatusFileSize
new 3173595-2.patch653 bytes
new 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 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 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 commented
Assigned: Unassigned » meena.bisht
Issue summary: View changes
StatusFileSize
new Screenshot 2020-10-08 at 11.56.05 AM.png267.99 KB
new 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 commented
Status: Needs work » Needs review
sulfikar_s’s picture

sulfikar_s 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 commented
Status: Needs review » Reviewed & tested by the community
quietone’s picture

quietone 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 commented
StatusFileSize
new 3173595-11.patch650 bytes

Hi,

I am updating patch as per #10 comments.

Please review the patch.

Thanks.

Pooja Ganjage’s picture

Pooja Ganjage commented
Status: Needs work » Needs review
Pooja Ganjage’s picture

Pooja Ganjage 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 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 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
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
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.