Part of meta-issue #2002650: [meta] improve maintainability by removing unused local variables

File /core/modules/block/lib/Drupal/block/Tests/BlockTestBase.php

Line 71: Unused local variable $manager

Files: 
CommentFileSizeAuthor
#14 drupal-core-remove-unused-local-variable-2081207-14.patch691 bytesboran
PASSED: [[SimpleTest]]: [MySQL] 58,932 pass(es).
[ View ]
#10 drupal-core-remove-unused-local-variable-2081207-10.patch748 bytesboran
FAILED: [[SimpleTest]]: [MySQL] 59,364 pass(es), 8 fail(s), and 4 exception(s).
[ View ]
#1 drupal-core-remove-unused-local-variable-2081207-1.patch652 bytessmiro
PASSED: [[SimpleTest]]: [MySQL] 58,093 pass(es).
[ View ]

Comments

StatusFileSize
new652 bytes
PASSED: [[SimpleTest]]: [MySQL] 58,093 pass(es).
[ View ]

Status:Active» Needs review

Status:Needs review» Reviewed & tested by the community

Variable safely removed from the method Drupal\block\Tests\BlockTestBase->setUp().

Changing status. Thank you for your patch @smiro

Status:Reviewed & tested by the community» Needs work

+++ b/core/modules/block/lib/Drupal/block/Tests/BlockTestBase.php
@@ -68,7 +68,6 @@ function setUp() {
-    $manager = $this->container->get('plugin.manager.block');
     $instances = config_get_storage_names_with_prefix('plugin.core.block.' . $default_theme);

This must just be wrong, since plugin.core.block is no longer part of core.
So let's actually fix this

A grep -R shows that plugin.core.block exists nowhere in the tree as you note.

core/modules/block/config/schema/block.schema.yml contains
block.block.*.*:

There is one other place where config_get_storage_names_with_prefix() is called on blocks
core/modules/block/lib/Drupal/block/Tests/NewDefaultThemeBlocksTest.php
$default_prefix = "block.block.$default_theme";
$new_prefix = "block.block.$new_theme";
$default_block_names = config_get_storage_names_with_prefix($default_prefix);

Which would hint that what is needed is:
$instances = config_get_storage_names_with_prefix('block.block.' . $default_theme);

The basic functionality was added in https://drupal.org/node/1874598.

Next: changed the instances line as noted above, will run some tests and get back to you.

Assigned:Unassigned» RunePhilosof

I'll look into it

the line we are talking about is around line 71 in setUp() of the class BlockTestBase
new: $instances = config_get_storage_names_with_prefix('block.block.' . $default_theme);
old: $instances = config_get_storage_names_with_prefix('plugin.core.block.' . $default_theme);

BlockTest inherits from BlockTestBase and is a test with the description 'Tests basic block functionality.'.

Now, I ran this test (enable the test) module and it does not report an error with either the new or old code.
So, how can we get the line of code to execute, is it ever used?

It should definitely be block.block. on that line.
I have tested it by including the blocks configuration from the minimal profile in the testing profile.
And that code removes the added blocks from the configuration.

However, another test failed even though the blocks were removed by the setup.
The failing test was testBlockRehash.
It fails on first line:

<?php
\Drupal::moduleHandler()->install(array('block_test'));
?>

Drupal\Component\Plugin\Exception\PluginException: The plugin (test_html_id) did not specify an instance class. in Drupal\Component\Plugin\Factory\DefaultFactory::getPluginClass() (line 62 of /var/www/drupal8/core/lib/Drupal/Component/Plugin/Factory/DefaultFactory.php).

StatusFileSize
new748 bytes
FAILED: [[SimpleTest]]: [MySQL] 59,364 pass(es), 8 fail(s), and 4 exception(s).
[ View ]

New patch rollled that includes the oneline from #1, plus the argument correction to config_get_storage_names_with_prefix().
Hopefully we are ready to go.

The issue noted in #9 whereby adding blocks to the testing profile causes block tests to fail should be handled in a separate issue.
@RunePhilosof : the troubleshooting together at the prague sprint was fun today, nice to have met you in person :-)

Title:Remove Unused local variable $manager from /core/modules/block/lib/Drupal/block/Tests/BlockTestBase.phpRemove obsolete test code from /core/modules/block/lib/Drupal/block/Tests/BlockTestBase.php

+++ b/core/modules/block/lib/Drupal/block/Tests/BlockTestBase.php
@@ -68,8 +68,7 @@ function setUp() {
     );
     $default_theme = variable_get('theme_default', 'stark');
-    $manager = $this->container->get('plugin.manager.block');
-    $instances = config_get_storage_names_with_prefix('plugin.core.block.' . $default_theme);
+    $instances = config_get_storage_names_with_prefix('block.block.' . $default_theme);
     foreach ($instances as $plugin_id) {
       \Drupal::config($plugin_id)->delete();
     }

Actually, all of these lines can be deleted. They are not used anymore. If they had, it wouldn't have worked with plugin.core.block anyway

If there were blocks configured by default in the test profile, would it not be best to leave that code to delete any blocks that are present before tests start?
But its not my area of expertise ... I'll re-roll and delete the following if you wish (please confirm)
$manager = $this->container->get('plugin.manager.block');
$manager = $this->container->get('plugin.manager.block');
$instances = config_get_storage_names_with_prefix('plugin.core.block.' . $default_theme);
foreach ($instances as $plugin_id) {
\Drupal::config($plugin_id)->delete();
}

It would be better to do that, but it is not necessary, or it would have been broken ever since we renamed plugin.core.block

Yes, please remove everything after

    $this->regions = array(
      'header',
      'sidebar_first',
      'content',
      'sidebar_second',
      'footer',
    );

StatusFileSize
new691 bytes
PASSED: [[SimpleTest]]: [MySQL] 58,932 pass(es).
[ View ]

New patch with lines removed according to #13

Status:Needs work» Needs review

Status:Needs review» Reviewed & tested by the community

Thanks

I have created a follow up issue on the test that failed when blocks are added in the config/block.block.stark.*.yml and then removed in the setUp().
#2103569: Adding blocks to the testing profile and removing causes errors in the tests

Status:Reviewed & tested by the community» Fixed

Yay, less code!

Committed and pushed to 8.x. Thanks!

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