Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Part of meta-issue #2002650: [meta, no patch] improve maintainability by removing unused local variables
File /core/modules/block/lib/Drupal/block/Tests/BlockTestBase.php
Line 71: Unused local variable $manager
Comment | File | Size | Author |
---|---|---|---|
#14 | drupal-core-remove-unused-local-variable-2081207-14.patch | 691 bytes | boran |
#10 | drupal-core-remove-unused-local-variable-2081207-10.patch | 748 bytes | boran |
#1 | drupal-core-remove-unused-local-variable-2081207-1.patch | 652 bytes | smira |
Comments
Comment #1
smira CreditAttribution: smira commentedComment #2
smira CreditAttribution: smira commentedComment #3
lorique CreditAttribution: lorique commentedVariable safely removed from the method Drupal\block\Tests\BlockTestBase->setUp().
Changing status. Thank you for your patch @smiro
Comment #4
tim.plunkettThis must just be wrong, since plugin.core.block is no longer part of core.
So let's actually fix this
Comment #5
boran CreditAttribution: boran commentedA 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.
Comment #6
RunePhilosof CreditAttribution: RunePhilosof commentedComment #7
RunePhilosof CreditAttribution: RunePhilosof commentedI'll look into it
Comment #8
boran CreditAttribution: boran commentedthe 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?
Comment #9
RunePhilosof CreditAttribution: RunePhilosof commentedIt 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\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).
Comment #10
boran CreditAttribution: boran commentedNew 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 :-)
Comment #11
tim.plunkettActually, 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
Comment #12
boran CreditAttribution: boran commentedIf 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();
}
Comment #13
tim.plunkettIt 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
Comment #14
boran CreditAttribution: boran commentedNew patch with lines removed according to #13
Comment #15
mcrittenden CreditAttribution: mcrittenden commentedComment #16
tim.plunkettThanks
Comment #17
RunePhilosof CreditAttribution: RunePhilosof commentedI 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
Comment #18
webchickYay, less code!
Committed and pushed to 8.x. Thanks!