Problem/Motivation

In core/modules/node/tests/src/Functional/NodeRevisionsUiBypassAccessTest.php, testDisplayRevisionTab() initializes $node_storage, but it never uses it.

$node_storage = $this->container->get('entity_type.manager')->getStorage('node');

Proposed resolution

$node_storage should be removed.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Hardik_Patel_12 created an issue. See original summary.

Hardik_Patel_12’s picture

Assigned: Hardik_Patel_12 » Unassigned
Status: Active » Needs review
FileSize
748 bytes

Kindly review the patch.

johnwebdev’s picture

Status: Needs review » Reviewed & tested by the community

Yeah, looks right.

Hardik_Patel_12’s picture

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

apaderno’s picture

Issue summary: View changes
apaderno’s picture

That line is also present in the patch used on #2592325: Revision tab is not shown directly, which introduced the test.

+  /**
+   * Checks that the Revision tab is displayed correctly.
+   */
+  function testDisplayRevisionTab() {
+    $this->drupalPlaceBlock('local_tasks_block');
+
+    $this->drupalLogin($this->editor);
+    $node_storage = $this->container->get('entity.manager')->getStorage('node');
+
+    // Set page revision setting 'create new revision'. This will mean new
+    // revisions are created by default when the node is edited.
+    $type = NodeType::load('page');
+    $type->setNewRevision(TRUE);
+    $type->save();
paulocs’s picture

Status: Needs work » Reviewed & tested by the community

So as @kiamlaluno shows when the unused variable $node_storage was added in comment #8, I will let the issue as RTBC.

Cheers,
Paulo.

alexpott’s picture

Version: 9.1.x-dev » 8.9.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed 1859ee95b0 to 9.1.x and 80fbda803f to 9.0.x and e81a46dc49 to 8.9.x. Thanks!

Yep this variable has never been used. Backported to 8.9.x to keep tests aligned.

  • alexpott committed 1859ee9 on 9.1.x
    Issue #3155796 by Hardik_Patel_12, kiamlaluno: Remove Unused variable $...

  • alexpott committed 80fbda8 on 9.0.x
    Issue #3155796 by Hardik_Patel_12, kiamlaluno: Remove Unused variable $...

  • alexpott committed e81a46d on 8.9.x
    Issue #3155796 by Hardik_Patel_12, kiamlaluno: Remove Unused variable $...

Status: Fixed » Closed (fixed)

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