#2483433: Optimize CacheableMetadata::merge() + BubbleableMetadata::merge() landed. But several significant loose ends were left here, resulting in both dead and inconsistent code:

  1. Dead code: RendererInterface::mergeAttachments() was removed, but Renderer::mergeAttachments() was kept, despite having zero calls remaining.
  2. Inconsistent code: BubbleableMetadata::merge() now uses static::mergeAttachments(), but BubbleableMetadata::addAttachments() still calls Renderer::mergeAttachments(), despite the interface no longer guaranteeing the presence of that method.
CommentFileSizeAuthor
#23 follow_up_for_2483433-2505171-23.patch914 bytesneclimdul
#10 interdiff.txt676 bytesWim Leers
#10 2505171-10.patch5.97 KBWim Leers
#7 interdiff.txt3.54 KBWim Leers
#7 2505171-7.patch5.94 KBWim Leers
#3 2505171-3.patch2.45 KBWim Leers
#1 2505171-1.patch2.45 KBWim Leers
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Active » Needs review
FileSize
2505171-1.patch2.45 KB

Status: Needs review » Needs work

The last submitted patch, 1: 2505171-1.patch, failed testing.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs work » Needs review
FileSize
2505171-3.patch2.45 KB
damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented

several significant loose ends were left here

I think this is over stating things slightly.

Looks like the reroll left that in, oops. The addAttachments method has zero test coverage, I think?

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Issue tags: +Needs tests

#4: I see how it can be read that way. I just meant to say that those things are significant enough to actually fix in a follow-up, i.e. they're not just typos that we could just leave there. Sorry for the poor wording :(

addAttachments method has zero test coverage, I think?

Yep, which is probably why this was missed. And for clarity: that's definitely a bigger problem than these loose ends.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented

Sure, np. So... do you think we should add some coverage here? or just do this and have another issue for comprehensive coverage? Either way, we might need another issue to make sure BubbleableMetadata is covered?

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers as a volunteer commented
Issue tags: -Needs tests
FileSize
2505171-7.patch5.94 KB
interdiff.txt3.54 KB

Updated test coverage.

IMO we should make \Drupal\Core\Render\BubbleableMetadata::mergeAttachments() protected, but that can happen in a follow-up.

yched’s picture

yched CreditAttribution: yched commented

IMO we should make \Drupal\Core\Render\BubbleableMetadata::mergeAttachments() protected, but that can happen in a follow-up

+1

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented 
  1. +++ b/core/tests/Drupal/Tests/Core/Render/BubbleableMetadataTest.php
@@ -98,6 +102,36 @@ public function providerTestMerge() {
+   * @covers ::addAttachments

    Also covers setAttachments indirectly too. 

  2. +++ b/core/tests/Drupal/Tests/Core/Render/BubbleableMetadataTest.php
@@ -98,6 +102,36 @@ public function providerTestMerge() {
+   * This only tests at a high level, because it reuses existing logic. Detailed

    Great docs. 

  3. +++ b/core/tests/Drupal/Tests/Core/Render/BubbleableMetadataTest.php
@@ -98,6 +102,36 @@ public function providerTestMerge() {
+    $test = $initial;

    Why do we need this? The same object will be used anyway here?

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
FileSize
2505171-10.patch5.97 KB
interdiff.txt676 bytes

#9.3: we don't need it, but it's to make it clear that we are given an initial object, and then modify it. So I first assign it to a variable and then modify it. That's all :) If you're not convinced, happy to change it. But do you then have a suggestion for a better variable name?

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
Status: Needs review » Reviewed & tested by the community

Self documenting :) Fair enough.

I think this looks good in that case.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 10: 2505171-10.patch, failed testing.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs work » Reviewed & tested by the community

Testbotfail.

cURL error 28: See http://curl.haxx.se/libcurl/c/libcurl-errors.html
→

CURLE_OPERATION_TIMEDOUT (28)

Operation timeout. The specified time-out period was reached according to the conditions.

Wim Leers queued 10: 2505171-10.patch for re-testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 10: 2505171-10.patch, failed testing.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs work » Reviewed & tested by the community

Now we have 2 failures. WTF is this? Testbotfail or not? IDK.

Wim Leers queued 10: 2505171-10.patch for re-testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 10: 2505171-10.patch, failed testing.

Wim Leers queued 10: 2505171-10.patch for re-testing.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs work » Reviewed & tested by the community

Came back green again, back to RTBC.

(Everything after #11 is due to testbot flakiness… :()

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks!

  • catch committed 8114a72 on 8.0.x 
    Issue #2505171 by Wim Leers: Follow-up for #2483433
neclimdul’s picture

neclimdul
He/Him
CreditAttribution: neclimdul commented
Status: Fixed » Needs review
FileSize
follow_up_for_2483433-2505171-23.patch914 bytes

Command line phpunit runner is failing:

There was 1 failure:

1) Warning
The data provider specified for Drupal\Tests\Core\Render\BubbleableMetadataTest::testMergeAttachmentsHtmlHeadLinkMerging is invalid.
Method providerTestMergeAttachementsHtmlHeadLinkMerging does not exist

FAILURES!
Tests: 7998, Assertions: 14089, Failures: 1, Skipped: 65.
Build step 'Execute shell' marked build as failure
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Priority: Normal » Critical
Status: Needs review » Reviewed & tested by the community

That's annoying that run-tests.sh and the web runner show what should be phpunit fails as passes - do we have an open issue for this?

RTBC pending the bot. Also critical.

neclimdul’s picture

neclimdul
He/Him
CreditAttribution: neclimdul commented

I've personally not seen failures like this. Generally these failures are because testbot is running phpunit tests as their own process instead of as a bulk run. This is the first failure like this I've seen.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks!

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Priority: Critical » Normal

Back to normal. If this hadn't been patched 5 minutes after the commit I'd have reverted normally.

  • catch committed 848d6b8 on 8.0.x 
    Issue #2505171 by neclimdul: Follow-up for follow-up for #2483433 (...
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

I've seen this problem many times before. The discrepancies between test runners is quite annoying.

Status: Fixed » Closed (fixed)

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