Problem/Motivation

#2477223: Refactor _drupal_add_html_head, drupal_get_html_head, _drupal_add_html_head_link into the attachments processor, remove from common.inc. needed a few re-rolls.

In that process, some new files got left out of the new patches, namely tests.

Proposed resolution

Add them back.

Remaining tasks

User interface changes

API changes

Data model changes

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because it's a follow-up to an issue that made it in without tests.
Issue priority Major because nothing's broken, but ongoing refactoring of this system will need tests to prevent regression.
Unfrozen changes Unfrozen because this issue adds tests (that were already RTBC, no less..)
Prioritized changes Prioritized in support of refactoring deprecated code: #2554771: Remove deprecated drupal_process_attached()
CommentFileSizeAuthor
#14 interdiff.txt2.52 KBmile23
#14 2571427_14.patch14.29 KBmile23
#2 2571427_2.patch14.3 KBmile23

Comments

Mile23 created an issue. See original summary.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Active » Needs review
Parent issue: » #2477223: Refactor _drupal_add_html_head, drupal_get_html_head, _drupal_add_html_head_link into the attachments processor, remove from common.inc.
StatusFileSize
new 2571427_2.patch14.3 KB

And here they are.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Title: Follow up: Tests got rerolled out of existence. » HtmlResponseAttachmentsProcessor tests misplaced during reroll
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Reviewed & tested by the community

Already was RTBC before, so back.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 2: 2571427_2.patch, failed testing.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Needs work » Reviewed & tested by the community

Is that an unrelated fail?

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

It must be.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Priority: Normal » Major
Issue summary: View changes
mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented

#2554771: Remove deprecated drupal_process_attached() and #2566619: Deprecate drupal_http_header_attributes() postponed on this.

ianthomas_uk’s picture

ianthomas_uk
he/him
Primary language English
Location Brighton, UK
commented
Issue tags: +rc deadline

Tagging because of the two issues this blocks

Mile23 queued 2: 2571427_2.patch for re-testing.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented

Sure would love to be able to follow through on those other issues. :-)

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work 
  1. +++ b/core/modules/system/src/Tests/Render/HtmlResponseAttachmentsTest.php
@@ -0,0 +1,175 @@
+    // Now repeat all of the preceeding using drupal_process_attached().

    I think this should be preceding 

  2. +++ b/core/modules/system/tests/modules/render_attached_test/src/Controller/TestController.php
@@ -0,0 +1,121 @@
+    \drupal_process_attached($this->teapotHeaderStatus());
...
+    \drupal_process_attached($this->header());
...
+    \drupal_process_attached($this->head());
...
+    \drupal_process_attached($this->feed());

    Why are we bothering with a leading backslash - we don't do this anywhere else... for example: 

  3. +++ b/core/modules/system/tests/modules/render_attached_test/src/Plugin/Block/DrupalProcessAttachedBlock.php
@@ -0,0 +1,52 @@
+    drupal_process_attached($attached);

    Here, for example.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Needs work » Needs review
StatusFileSize
new 2571427_14.patch14.29 KB
new interdiff.txt2.52 KB

Thanks.

And... dun.

Status: Needs review » Needs work

The last submitted patch, 14: 2571427_14.patch, failed testing.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Needs work » Needs review

Grr. Testbot.

Wim Leers queued 14: 2571427_14.patch for re-testing.

ianthomas_uk’s picture

ianthomas_uk
he/him
Primary language English
Location Brighton, UK
commented
Status: Needs review » Reviewed & tested by the community

interdiff is accurate and fixes everything in #13

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed 9d3c2c5 and pushed to 8.0.x. Thanks!

  • alexpott committed 9d3c2c5 on 8.0.x 
    Issue #2571427 by Mile23: HtmlResponseAttachmentsProcessor tests...

Status: Fixed » Closed (fixed)

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