Support from Acquia helps fund testing for Drupal Acquia logo

Comments

smira’s picture

smira’s picture

Status: Active » Needs review
lorique’s picture

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

tim.plunkett’s picture

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

boran’s picture

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.

RunePhilosof’s picture

Assigned: Unassigned » RunePhilosof
RunePhilosof’s picture

I'll look into it

boran’s picture

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?

RunePhilosof’s picture

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:

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

boran’s picture

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 :-)

tim.plunkett’s picture

Title: Remove Unused local variable $manager from /core/modules/block/lib/Drupal/block/Tests/BlockTestBase.php » Remove 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

boran’s picture

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();
}

tim.plunkett’s picture

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',
    );
boran’s picture

New patch with lines removed according to #13

mcrittenden’s picture

Status: Needs work » Needs review
tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Thanks

RunePhilosof’s picture

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

webchick’s picture

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.