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.
Problem/Motivation
In core/tests/Drupal/Tests/Core/Render/BubbleableMetadataTest.php testMergeAttachmentsLibraryMerging()
initializes $settings_two['moduleName']['thingiesOnPage']['id1']
, which is then never used.
Proposed resolution
Remove $settings_two['moduleName']['thingiesOnPage']['id1']
.
// adds the exact same settings twice and hence tests idempotency, the
// second adds *almost* the same settings twice: the second time, some
// values are altered, and some key-value pairs are added.
- $settings_two['moduleName']['thingiesOnPage']['id1'] = [];
$this->assertSame($settings_one, $merged['drupalSettings']['commonTestRealWorldIdentical']);
$expected_settings_two = $settings_two_a;
$expected_settings_two['moduleName']['ui'][0] = 'button D';
Comment | File | Size | Author |
---|---|---|---|
#6 | drupal-remove-unused-variable-3158286-6.patch | 1.06 KB | apaderno |
unsused-variable-removed.patch | 880 bytes | Hardik_Patel_12 | |
Comments
Comment #2
apadernoReading the comment before the assignment, I wonder if there is simply a typo in the variable name.
Maybe the line should simply be the following one.
Comment #3
apadernoThis is harder to track.
The code in core/tests/Drupal/Tests/Core/Render/BubbleableMetadataTest.php that includes the
$settings_two['moduleName']['thingiesOnPage']['id1'] = [];
line (truly,$settings_two['moduleName']['thingiesOnPage']['id1'] = array();
) has been added in #2483433: Optimize CacheableMetadata::merge() + BubbleableMetadata::merge(), but the patch in that issue simply moved existing code from theMergeAttachmentsTest
class to theBubbleableMetadataTest
class where it is now.I will try to understand what code was used before that patch, after I find a commit that still contains the
MergeAttachmentsTest
class.Comment #4
apaderno#2382557: Change JS settings into a separate asset type moved the
$settings_two['moduleName']['thingiesOnPage']['id1'] = array();
line from core/modules/system/src/Tests/Common/JavaScriptTest.php to core/modules/system/src/Tests/Common/MergeAttachmentsTest.php.(I apologize for posting multiple comments, but it's rather long to find out when a single line has been added in a file with
git log --full-history
, especially when that line keeps being moved from file to file.)Comment #5
apadernoFinally, this is the code used in commit 8c68491896ce0a4a52dce9f8de94d23623c70fef.
$settings_two
is changed right before comparing it with$parsed_settings['commonTestRealWorldAlmostIdentical']
. That doesn't happen with the currently used code.Comparing the code from that commit and the currently used code, I conclude the currently used code should be the following one.
Comment #6
apadernoComment #7
longwave@kiamlaluno++ excellent git analysis, agree with all your findings here that this is a typo/refactoring error rather than simply an unused variable.
Comment #9
catchNice bit of archaeology.
Committed 836157c and pushed to 9.1.x. Thanks!