Problem/Motivation

In core/tests/Drupal/Tests/Core/Entity/EntityLinkTest.php file testToLink() initializes $expected, but never uses that variable.

Proposed resolution

Remove the unused $expected variable.

public function testToLink($entity_label, $link_text, $expected_text, $link_rel
     $route_name = $route_name_map[$link_rel];
     $entity_id = 'test_entity_id';
     $entity_type_id = 'test_entity_type';
-    $expected = '<a href="/test_entity_type/test_entity_id">' . $expected_text . '</a>';
 
     $entity_type = $this->createMock('Drupal\Core\Entity\EntityTypeInterface');
     $entity_type->expects($this->once())
CommentFileSizeAuthor
unsused-variable-removed.patch722 byteshardik_patel_12

Comments

Hardik_Patel_12 created an issue. See original summary.

avpaderno’s picture

Issue summary: View changes

I added the link to the documentation page for the test method. (It's for Drupal 9.0.x, but the code is the same Drupal 9.1.x uses.)

avpaderno’s picture

Given that the test method uses the following code, it's probable the $expect variable was used to build the link; the code has been changed, but the variable has never been removed.

  $expected_link = Link::createFromRoute($expected_text, $route_name, [
    $entity_type_id => $entity_id,
  ], [
    'entity_type' => $entity_type_id,
    'entity' => $entity,
  ] + $link_options);
avpaderno’s picture

As side note, should this issue be used for a single test class, or extended to all the test classes in the Drupal\Tests\Core\Entity namespace?

hardik_patel_12’s picture

@kiamlaluno thankyou for updating link in summary , i will update in remaining child issues very soon. We can create single issue for Drupal\Tests\Core\Entity namespace also but as parent issue is saying Identify each file that has an unused variable and make one issue per file.
So that's why creating issue single test class.

avpaderno’s picture

Status: Needs review » Reviewed & tested by the community

In that case, the patch removes all the unused variables present in that file.

alexpott’s picture

Status: Reviewed & tested by the community » Closed (duplicate)
Related issues: +#3106216: Remove unused variables from core

Thank you for your work on cleaning up Drupal core's code style!

In order to fix core coding standards in a maintainable way, all our coding standards issues should be done on a per-rule basis across all of core, rather than fixing standards in individual modules or files. We should also separate fixes where we need to write new documentation from fixes where we need to correct existing standards. This all should be done as part of #2571965: [meta] Fix PHP coding standards in core, stage 1. A good place to for unused variables is #3106216: Remove unused variables from core.

For background information on why we usually will not commit coding standards fixes that aren't scoped in that way, see the core issue scope guidelines, especially the note about coding standards cleanups. That document also includes numerous suggestions for scoping issues including documentation coding standards cleanups.

Contributing to the overall plan above will help ensure that your fixes for core's coding standards remain in core the long term.

alexpott’s picture

Status: Closed (duplicate) » Needs work

In discussion with xim, catch and larowlan, my earlier comment is incorrect. We should handle each unused variable on its own merit and do the work to work out why it is not used.

This can point to broken code or incomplete testing see #3157369: Use unused variable $filters from DateTimeSchemaTest for example. A useful tool for this is git log -S “SOME TEXT” which will search git commits for matching text to find out when the variable might have become unused. Without doing the work to show why the variable is unused the patch will not be committed. Also git blame can be useful as well.

avpaderno’s picture

Basing on git blame, the following code was added from #2606398: Add EntityInterface::toUrl() and EntityInterface::toLink() and mark urlInfo(), url() and link() as deprecated.

    $language = new Language(['id' => 'es']);
    $link_options += ['language' => $language];
    $this->languageManager->expects($this->any())
      ->method('getLanguage')
      ->with('es')
      ->willReturn($language);

    $route_name_map = [
      'canonical' => 'entity.test_entity_type.canonical',
      'edit-form' => 'entity.test_entity_type.edit_form',
    ];
    $route_name = $route_name_map[$link_rel];
    $entity_id = 'test_entity_id';
    $entity_type_id = 'test_entity_type';
    $expected = '<a href="/test_entity_type/test_entity_id">' . $expected_text . '</a>';

The patch on that issue also added the following code.

    $expected_link = Link::createFromRoute(
      $expected_text,
      $route_name,
      [$entity_type_id => $entity_id],
      ['entity_type' => $entity_type_id, 'entity' => $entity] + $link_options
    );

    $result_link = $entity->toLink($link_text, $link_rel, $link_options);
    $this->assertEquals($expected_link, $result_link);

I am still convinced that the patch was written to use $expected as the other tests, but then used $expected_link.

rajandro’s picture

I am working on this.

rajandro’s picture

Status: Needs work » Needs review

I have rechecked the code EntityLinkTest.php and the git history. The variable ($expected) is unused form the beginning of the code addition. I am moving this to NR as everything looks good to me.

longwave’s picture

Status: Needs review » Reviewed & tested by the community

I agree with the above findings, I think this can just be removed as the $expected_link check is the same, this line seems leftover during development and committed by mistake.

  • catch committed b045d78 on 9.1.x
    Issue #3158277 by Hardik_Patel_12, kiamlaluno, alexpott, rajandro,...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed b045d78 and pushed to 9.1.x. Thanks!

Status: Fixed » Closed (fixed)

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