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';
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Hardik_Patel_12 created an issue. See original summary.

apaderno’s picture

Issue summary: View changes

Reading the comment before the assignment, I wonder if there is simply a typo in the variable name.

    // Test whether the two real world cases are handled correctly: the first
    // 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'] = [];

Maybe the line should simply be the following one.

$expected_settings_two['moduleName']['thingiesOnPage']['id1'] = [];
apaderno’s picture

This 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 the MergeAttachmentsTest class to the BubbleableMetadataTest 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.

apaderno’s picture

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

apaderno’s picture

Status: Needs review » Needs work

Finally, this is the code used in commit 8c68491896ce0a4a52dce9f8de94d23623c70fef.

    // Test whether the two real world cases are handled correctly.
    $settings_two['moduleName']['thingiesOnPage']['id1'] = array();
    $this->assertIdentical($settings_one, $parsed_settings['commonTestRealWorldIdentical'], '_drupal_add_js handled real world test case 1 correctly.');
    $this->assertEqual($settings_two, $parsed_settings['commonTestRealWorldAlmostIdentical'], '_drupal_add_js handled real world test case 2 correctly.');

$settings_two is changed right before comparing it with $parsed_settings['commonTestRealWorldAlmostIdentical']. That doesn't happen with the currently used code.

    // Test whether the two real world cases are handled correctly: the first
    // 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';
    $expected_settings_two['moduleName']['ui'][1] = 'button E';
    $expected_settings_two['moduleName']['ui'][2] = 'button C';
    $expected_settings_two['moduleName']['magical flag'] = 3.14;
    $expected_settings_two['moduleName']['thingiesOnPage']['id2'] = [];
    $this->assertSame($expected_settings_two, $merged['drupalSettings']['commonTestRealWorldAlmostIdentical']);

Comparing the code from that commit and the currently used code, I conclude the currently used code should be the following one.

    // Test whether the two real world cases are handled correctly: the first
    // 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.
    $this->assertSame($settings_one, $merged['drupalSettings']['commonTestRealWorldIdentical']);
    $expected_settings_two = $settings_two_a;
    $expected_settings_two['moduleName']['thingiesOnPage']['id1'] = [];
    $expected_settings_two['moduleName']['ui'][0] = 'button D';
    $expected_settings_two['moduleName']['ui'][1] = 'button E';
    $expected_settings_two['moduleName']['ui'][2] = 'button C';
    $expected_settings_two['moduleName']['magical flag'] = 3.14;
    $expected_settings_two['moduleName']['thingiesOnPage']['id2'] = [];
    $this->assertSame($expected_settings_two, $merged['drupalSettings']['commonTestRealWorldAlmostIdentical']); 
apaderno’s picture

Status: Needs work » Needs review
FileSize
1.06 KB
longwave’s picture

Status: Needs review » Reviewed & tested by the community

@kiamlaluno++ excellent git analysis, agree with all your findings here that this is a typo/refactoring error rather than simply an unused variable.

  • catch committed bf86869 on 9.1.x
    Issue #3158286 by kiamlaluno, Hardik_Patel_12, longwave: Unused local...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Nice bit of archaeology.

Committed 836157c and pushed to 9.1.x. Thanks!

Status: Fixed » Closed (fixed)

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