Unused local variables from ScaffoldTest.php file

I take the code was supposed to be similar to the following one. (The second parameter of scaffoldSut() is $is_link: public function scaffoldSut($fixture_name, $is_link = FALSE, $relocated_docroot = TRUE).)

$is_link = FALSE;
$result = $this->scaffoldSut($fixture_name, $is_link, FALSE);

Given that scaffoldSut() is called only once from that method, it doesn't make sense to assign a constant to a variable and pass the variable to the method.

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.

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.

The following code has been added in #2982684: Add a composer scaffolding plugin to core.

+  /**
+   * Try to scaffold a project that allows a project with no scaffold files.
+   */
+  public function testProjectThatScaffoldsEmptyProject() {
+    $fixture_name = 'project-allowing-empty-fixture';
+    $is_link = FALSE;
+    $result = $this->scaffoldSut($fixture_name, FALSE, FALSE);
+    $this->assertContains('The allowed package fixtures/empty-fixture does not provide a file mapping for Composer Scaffold', $result->scaffoldOutput());
+    $this->assertCommonDrupalAssetsWereScaffolded($result->docroot(), FALSE);
+  }

I didn't find anything in that issue that would make think the intention was to first call scaffoldSut() as $this->scaffoldSut($fixture_name, FALSE, FALSE) and then as $this->scaffoldSut($fixture_name, TRUE, FALSE).

Since the full method has been added, $is_link = FALSE; is not a left-over from a previous commit.

Since there are two methods that have $is_link as parameter, maybe the intention was effectively calling first $this->scaffoldSut($fixture_name, FALSE, FALSE) and then $this->scaffoldSut($fixture_name, TRUE, FALSE)

Drupal\Tests\Composer\Plugin\Scaffold\Functional\ScaffoldTest::testProjectThatScaffoldsEmptyProject
Failed asserting that false is identical to true.

I guess this means the patch in #6 is wrong and that the first patch is the correct one.

Hello @all,

I agree with @kiamlaluno with what he said in comment #5. Patch #1 (unsused-variable-removed.patch), looks good to me.

Set to RTBC

The correct patch needs to be the most recent patch on the issue - because anything else is confusing and the rtbc re-tester does not work. Also I think it is worth asking @mile23 / @greg.1.anderson what the intentions were here. I've pinged them.

So here is the correct patch.
Let's see what @mile23 / @greg.1.anderson will say.

I didn't attach the patch in the comment above so here it is.

Composer changes is out of scope and wrong

My mistake. Follow a new patch!

I just skimmed this but #2 is correct and #13 is fine.

The tests used to test the link / no link varieties via data providers, but so much redundant testing took extra time, and was not necessary. Having inline $is_link variables was an intermediate step when considering whether to remove a data provider from a test. The intention was to remove all of these, but looks like one was missed.

The test failure is unrelated. Not sure what the policy is on committing patches with unrelated failures, but YOLO, here I go RTBC'ing this. Ping me if you want me to re-RTBC it after the test passes (presumably should be able to just re-run after the main branch is fixed).

Committed and pushed 5dd97b3aca to 9.1.x and af45edfff4 to 9.0.x and f7a9cf8eaa to 8.9.x. Thanks!

Backported to 8.9.x as this is test.