Parent issue: #2205673: [meta] Remove all @deprecated functions marked "remove before 8.0"

Problem/Motivation

These functions are marked as deprecated for removal before the 8.0.0 release:

  • _drupal_add_html_head()
  • drupal_get_html_head()
  • _drupal_add_html_head_link()

They are mainly only called by drupal_process_attached(). drupal_get_html_head() is called in a few other places: book.module and AddFeedTest.

They are also currently used as backward-compatibility glue within HtmlResponseAttachmentsProcessor, which will replace them in this issue.

Proposed resolution

  • Remove usage of _drupal_add_html_head, drupal_get_html_head and _drupal_add_html_head_link
  • Remove _drupal_add_html_head, drupal_get_html_head and _drupal_add_html_head_link from common.inc

Remaining tasks

User interface changes

API changes

Data model changes

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because we're removing deprecated functions.
Issue priority Major because it is not critical, but does have significant impact and is important by community consensus.
Prioritized changes Prioritized because we're removing code marked @deprecated for 8.0.0.
Disruption Minimal disruption due to backwards-compatibility.
CommentFileSizeAuthor
#116 2477223_116.patch26.14 KBianthomas_uk
#100 interdiff.txt720 bytesMile23
#100 2477223_100.patch40.46 KBMile23
#95 2477223_95.patch40.31 KBMile23
#92 interdiff.txt623 bytesMile23
#92 2477223_90.patch40.95 KBMile23
#89 interdiff.txt1.95 KBWim Leers
#89 2477223_89.patch40.71 KBWim Leers
#88 interdiff.txt617 bytesMile23
#88 2477223_88.patch41.21 KBMile23
#83 interdiff.txt747 bytesWim Leers
#83 2477223_83.patch41.21 KBWim Leers
#80 interdiff.txt2.51 KBMile23
#80 2477223_80.patch40.56 KBMile23
#74 interdiff.txt1.8 KBMile23
#74 2477223_74_rendercontext.patch40.61 KBMile23
#72 interdiff.txt449 bytesMile23
#72 2477223_72_rendercontext.patch40.35 KBMile23
#67 interdiff_65.txt17.27 KBMile23
#67 interdiff_static_to_rendercontext.txt2.18 KBMile23
#67 2477223_67_static.patch41.43 KBMile23
#67 2477223_67_rendercontext.patch40.34 KBMile23
#65 interdiff.txt3.21 KBMile23
#65 2477223_65.patch37.97 KBMile23
#60 2477223_60.patch35.71 KBMile23
#59 interdiff.txt4.92 KBMile23
#59 2477223_59.patch35.75 KBMile23
#56 interdiff.txt3.91 KBMile23
#56 2477223_56.patch36.13 KBMile23
#47 interdiff.txt8.99 KBMile23
#47 2477223_47.patch34.64 KBMile23
#44 interdiff.txt1011 bytesMile23
#44 2477223_44.patch30.15 KBMile23
#42 2477223_42.patch30.83 KBMile23
#39 interdiff.txt2.69 KBMile23
#39 2477223_39.patch30.17 KBMile23
#37 interdiff.txt8.57 KBMile23
#37 2477223_37.patch29.33 KBMile23
#29 2477223_29.patch30.33 KBMile23
#28 interdiff.txt7.71 KBMile23
#28 2477223_28.patch19.61 KBMile23
#26 interdiff.txt1.87 KBMile23
#26 2477223_26.patch15.39 KBMile23
#23 interdiff.txt7.98 KBMile23
#23 2477223_23.patch13.52 KBMile23
#21 interdiff.txt5.6 KBMile23
#21 2477223_21.patch8.73 KBMile23
#16 2477223_16.patch8.01 KBMile23
#10 remove_from_common_inc-2477223-re-roll.patch5.84 KBjaimeguzman
#6 remove_from_common_inc-2477223-6.patch5.84 KBj2r
#2 remove_from_common_inc-2477223-1.patch4.19 KBpjbaert
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

a_thakur’s picture

Title: Remove _drupal_add_html_head, drupal_get_html_head and _drupal_add_html_head_link from common.inc » Remove _drupal_add_html_head, drupal_get_html_head and _drupal_add_html_head_link from common.inc and usage
Issue summary: View changes
pjbaert’s picture

Status: Active » Needs review
FileSize
4.19 KB

I removed _drupal_add_html_head, drupal_get_html_head and _drupal_add_html_head_link & usages from common.inc

Status: Needs review » Needs work

The last submitted patch, 2: remove_from_common_inc-2477223-1.patch, failed testing.

a_thakur’s picture

Assigned: a_thakur » Unassigned
j2r’s picture

Assigned: Unassigned » j2r
j2r’s picture

Assigned: j2r » Unassigned
Status: Needs work » Needs review
FileSize
5.84 KB

Removed _drupal_add_html_head, drupal_get_html_head and _drupal_add_html_head_link & usages of these functions.

- wrong patch. Please ignore.

j2r’s picture

Assigned: Unassigned » j2r
Status: Needs review » Needs work

The last submitted patch, 6: remove_from_common_inc-2477223-6.patch, failed testing.

j2r’s picture

Assigned: j2r » Unassigned
jaimeguzman’s picture

Status: Needs work » Needs review
Issue tags: -Novice
FileSize
5.84 KB

I try delete the function too, check the patch above, and doesn't considered the usage, please rollback this patch.
Also this issue has related with this bug: https://www.drupal.org/node/1338858#comment-9922134

function usage :

$ git grep _drupal_add_html_head
core/includes/common.inc:169:function _drupal_add_html_head($data = NULL, $key = NULL) {
core/includes/common.inc:194:  $elements = _drupal_add_html_head();
core/includes/common.inc:563:function _drupal_add_html_head_link($attributes, $header = FALSE) {
core/includes/common.inc:577:  _drupal_add_html_head($element, 'html_head_link:' . $attributes['rel'] . ':' . $href);
core/includes/common.inc:712:          call_user_func_array('_drupal_add_html_head', $args);
core/includes/common.inc:721:          call_user_func_array('_drupal_add_html_head_link', $args);
core/includes/common.inc:724:          call_user_func_array('_drupal_add_html_head_link', $args);
$git grep drupal_get_html_head
 core/includes/common.inc:193:function drupal_get_html_head($render = TRUE) 
 core/includes/theme.inc:1348:  $variables['head'] = drupal_get_html_head(FALSE);
 core/modules/book/book.module:481:  $variables['head'] = drupal_get_html_head();
 core/modules/system/src/Tests/Common/AddFeedTest.php:66:    $this->setRawContent(drupal_get_html_head());
$ git grep _drupal_add_html_head_link
core/includes/common.inc:563:function _drupal_add_html_head_link($attributes, $header = FALSE) {
core/includes/common.inc:721:          call_user_func_array('_drupal_add_html_head_link', $args);
core/includes/common.inc:724:          call_user_func_array('_drupal_add_html_head_link', $args);

Status: Needs review » Needs work

The last submitted patch, 10: remove_from_common_inc-2477223-re-roll.patch, failed testing.

JeroenT’s picture

Hmm,

_drupal_add_html_head() and drupal_add_html_head_link() are still used in drupal_process_attached(). I currently don't have a clue how we should replace this usages here.

Mile23’s picture

Working on drupal_process_attached() here: #2467759: Refactor drupal_process_attached() so it doesn't depend on drupal_add_http_header()

Ideally we'd refactor all of these away at the same time. :-)

andypost’s picture

So maybe better to split this issues on per function basis?

Wim Leers’s picture

Mile23’s picture

Title: Remove _drupal_add_html_head, drupal_get_html_head and _drupal_add_html_head_link from common.inc and usage » Refactor _drupal_add_html_head, drupal_get_html_head, _drupal_add_html_head_link into the attachments processor, remove from common.inc.
Status: Needs work » Needs review
Issue tags: +Needs tests, +Needs beta evaluation
FileSize
8.01 KB

This patch essentially moves the functionality of the global functions to HtmlResponseAttachmentsProcessor.

It passes some tests locally, but it's not finished. We still need to handle the 'feed' render type.

Also there aren't any good pre-existing tests that are easy to find. :-) Therefore I've added the need tests tag. The test we need is a number of render arrays which add head and link elements. We really should add a new test class, called something like HtmlResponseAttachmentsProcessorTest.

The patch also uses drupal_http_header_attributes() which isn't marked as deprecated. Refactoring it somewhere else should be a follow-up.

Wim Leers’s picture

Thanks for working on this! :)

Existing test coverage lives in \Drupal\Tests\Core\Render\BubbleableMetadataTest, but that's just for merging. So yeah, test coverage is pretty sad.


  1. +++ b/core/lib/Drupal/Core/Render/HtmlResponseAttachmentsProcessor.php
    @@ -107,13 +118,35 @@ public function processAttachments(AttachmentsInterface $response) {
    +      // a head element, but also as a Link: http header depending on settings
    

    s/head//
    s/http/HTTP/
    s/settings/options/

  2. +++ b/core/lib/Drupal/Core/Render/HtmlResponseAttachmentsProcessor.php
    @@ -107,13 +118,35 @@ public function processAttachments(AttachmentsInterface $response) {
    +      // http_header sections, so we must address it first.
    

    s/sections/keys of #attached/

  3. +++ b/core/lib/Drupal/Core/Render/HtmlResponseAttachmentsProcessor.php
    @@ -107,13 +118,35 @@ public function processAttachments(AttachmentsInterface $response) {
    +      if (isset($attached['html_head_link'])) {
    +        $link_attached = $this->processHtmlHeadLink($attached['html_head_link']);
    +        // Merge $link_attached so that its html_head and http_header
    +        // sections are present for further processing.
    +        $attached = array_merge($attached, $link_attached);
    +      }
    +      // Now we can process html_head.
    +      if (isset($attached['html_head'])) {
    +        $head = $this->processHtmlHead($attached['html_head']);
    +        // Invoke hook_html_head_alter().
    +        $this->moduleHandler->alter('html_head', $head);
    +        $variables['head'] = $head;
    +      }
    

    Agreed that this needs extra (well… *any* probably…) test coverage

  4. +++ b/core/lib/Drupal/Core/Render/HtmlResponseAttachmentsProcessor.php
    @@ -107,13 +118,35 @@ public function processAttachments(AttachmentsInterface $response) {
    +    // We have to get these elements back from the legacy system using
    +    // drupal_get_html_head().
         if (isset($placeholders['head'])) {
    -      $variables['head'] = drupal_get_html_head(FALSE);
    +      $variables['head'] = array_merge($variables['head'], drupal_get_html_head(FALSE));
         }
    

    This does more merging, so especially needs test coverage.

Status: Needs review » Needs work

The last submitted patch, 16: 2477223_16.patch, failed testing.

andypost’s picture

tests show that we have solid integration coverage

+1 on approach, suppose we need @deprecated issue on drupal_http_header_attributes asap

Mile23’s picture

If anyone wants to tackle this, please go ahead. Just assign yourself.

I'm pretty sure many of those failing tests in #16 are due to not using bubbling render contexts or whatever the terminology is.

Also there's the matter of KernelTestBase::render(), which calls drupal_process_attached(). :-)

Mile23’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
8.73 KB
5.6 KB

Still needs tests...

Passes some local tests, but the testbot will be our arbiter.

At this point, drupal_process_attached() is unneeded.

Many of the previous fails were due to some typo-level stuff. :-)

Status: Needs review » Needs work

The last submitted patch, 21: 2477223_21.patch, failed testing.

Mile23’s picture

Status: Needs work » Needs review
Issue tags: +Needs change record
FileSize
13.52 KB
7.98 KB

Getting closer...

Finally removed the functions this issue is about. :-)

html_head_link gets merged using the bubbleable thingie, resulting in a lot of happiness.

Marked drupal_process_attached() as deprecated, because it looks pretty silly sitting there empty. This should really be a follow up issue.

There's some strangeness around 'feed', because I think some code specifies arrays and some does not. Since the documentation on this stuff is incomplete, it's hard to say which is the right way. I noticed the difference in the taxonomy tests, so perhaps taxonomy module is doing things 'wrong.'

Definitely needs more documentation, and a change notice about using the render array and not needing to call drupal_process_attached().

Status: Needs review » Needs work

The last submitted patch, 23: 2477223_23.patch, failed testing.

Wim Leers’s picture

At this point, drupal_process_attached() is unneeded.

WOOOHOOOOOOOOOO!

Great work here :)

Mile23’s picture

Status: Needs work » Needs review
FileSize
15.39 KB
1.87 KB

Ok, so the only failure is BookTest, because I don't know enough about preprocessing for templates.

andypost’s picture

Mile23’s picture

A tiny bit of cleanup, some doc changes.

Mile23’s picture

A bit of rearranging, refactored the 'feed' processor to another method, added tests.

The feed test isn't very good, but my xpath-fu is no good.

Status: Needs review » Needs work

The last submitted patch, 29: 2477223_29.patch, failed testing.

Mile23’s picture

Status: Needs work » Needs review
FileSize
29.34 KB
3.06 KB

Thou Shalt Not Change The Order Of Feed Link Attributes. (Because the test uses regex.)

dawehner’s picture

Issue tags: +Needs tests
  1. +++ b/core/includes/common.inc
    @@ -628,22 +542,16 @@ function drupal_process_attached(array $elements) {
           switch ($callback) {
             case 'html_head':
    

    Is there no way to remove this bit of code? Can't we also unset the values?

  2. +++ b/core/includes/common.inc
    @@ -628,22 +542,16 @@ function drupal_process_attached(array $elements) {
               break;
    ...
               break;
    ...
             case 'http_header':
    

    Nitpick: you could remove the break statements ...

  3. +++ b/core/lib/Drupal/Core/Render/HtmlResponseAttachmentsProcessor.php
    @@ -219,4 +295,68 @@ protected function setHeaders(HtmlResponse $response, array $headers) {
    +        $href = '<' . SafeMarkup::checkPlain($attributes['href']) . '>;';
    

    We would use #2550945: Add Html::escape() in the future. What about adding a todo?

  4. +++ b/core/modules/book/book.module
    @@ -468,7 +468,9 @@ function template_preprocess_book_export_html(&$variables) {
    +  // @todo: Commenting out the following line is wrong.
    +  // @see https://www.drupal.org/node/2477223
    +//  $variables['head'] = drupal_get_html_head();
    

    Can we remove that now? Just curious

  5. +++ b/core/modules/system/src/Tests/Common/AddFeedTest.php
    @@ -61,9 +61,15 @@ function testBasicFeedAddNoTitle() {
    +      $build, '', $this->container->get('theme.manager')->getActiveTheme()->getName()
    

    Did you considered to use \Drupal::theme() instead?

Wim Leers’s picture

  1. +++ b/core/includes/common.inc
    --- a/core/lib/Drupal/Core/Render/AttachmentsResponseProcessorInterface.php
    +++ b/core/lib/Drupal/Core/Render/AttachmentsResponseProcessorInterface.php
    

    Great docs updates here. :)

  2. +++ b/core/lib/Drupal/Core/Render/HtmlResponseAttachmentsProcessor.php
    @@ -10,11 +10,22 @@
    + * the render array, for HTML responses. It replaces drupal_process_attached()
    + * and drupal_get_html_head() from Drupal 7.
    

    I think the "it replace […]" phrase can be removed here; we never write Drupal 7 comparisons in our documentation.

  3. +++ b/core/lib/Drupal/Core/Render/HtmlResponseAttachmentsProcessor.php
    @@ -99,29 +120,65 @@ public function processAttachments(AttachmentsInterface $response) {
    +    // Since mergeBubbleableMetadata() needs full render arrays, we must set one
    +    // up here.
    +    $attached = [];
    +    $attached['#attached'] = $response->getAttachments();
    ...
    +        $attached = $this->renderer->mergeBubbleableMetadata(
    ...
    +        $attached = $this->renderer->mergeBubbleableMetadata(
    

    I'd like to avoid this here. Let's at the very least use \Drupal\Core\Render\BubbleableMetadata::merge() directly, perhaps even \Drupal\Core\Render\BubbleableMetadata::mergeAttachments().

    Then we avoid the whole "set up full render array" dance :)

  4. +++ b/core/lib/Drupal/Core/Render/HtmlResponseAttachmentsProcessor.php
    @@ -215,8 +272,102 @@ protected function setHeaders(HtmlResponse $response, array $headers) {
    +  ¶
    

    Nit: Trailing spaces.

  5. +++ b/core/lib/Drupal/Core/Render/HtmlResponseAttachmentsProcessor.php
    @@ -215,8 +272,102 @@ protected function setHeaders(HtmlResponse $response, array $headers) {
    +      list ($data, $key) = $item;
    

    s/list (/list(/

  6. +++ b/core/modules/system/src/Tests/Render/HtmlResponseAttachmentsTest.php
    @@ -0,0 +1,46 @@
    +   * Test rendering of ['#attached'].
    +   */
    +  public function testAttached() {
    

    s/attached/attachments/

Wim Leers’s picture

Wow, WTF did d.o do there!? It repeated the patch from #29, mysteriously, magically, scarily!? Opened #2552405: A patch uploaded by user X in comment N mysteriously appeared in a comment by user Y in comment N+4 after cross-posting with user Z for that.

Mile23’s picture

Status: Needs review » Needs work

@dawehner #32.1: "Is there no way to remove this bit of code? Can't we also unset the values?"

I'm not sure what to do with drupal_process_attached(). It should be deprecated before 8.0.0 and just removed, but there's rules and stuff about deprecation, so I've left the function mostly as-is so we can have this conversation.

Also, there's the matter of throwing an exception when you have a bad #attached key. Is that a behavior that needs to be replicated?

#32.4: There's another issue about that: #1338858: Include theme print stylesheets in Book printer friendly export. This one is really postponed on that one.

#32.5: No, because we really shouldn't be doing \Drupal in tests, tho of course it's a conversation: #2066993: Use \Drupal consistently in tests

#33.3: \Drupal\Core\Render\BubbleableMetadata::mergeAttachments() seems kind of obvious. :-)

The last submitted patch, 29: 2477223_29.patch, failed testing.

Mile23’s picture

Status: Needs work » Needs review
FileSize
29.33 KB
8.57 KB

This patch collapses drupal_process_attached() to it's leftover behavior.

Switched to BubbleableMetadata::mergeAttachments(), though it seems kind of weird to use statics and not the renderer interface.

Addressed all the nits. :-)

See #35 for other decisions.

...Oops, missed the inline docs about mergeAttachments().

Status: Needs review » Needs work

The last submitted patch, 37: 2477223_37.patch, failed testing.

Mile23’s picture

Priority: Normal » Major
Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs beta evaluation
FileSize
30.17 KB
2.69 KB

Fixed failing test, which proves that we needed the exception if bad #attached types are passed in.

Fixed minor inline comment problems.

Still waiting on #1338858: Include theme print stylesheets in Book printer friendly export (or even #1843718: Remove "export" functionality from Book module, apply to all node types) and then we can reroll this.

andypost’s picture

I'm still sure we need to remove this function because it does nothing now
Right now no idea how to split this issue but book issue could depend on it

+++ b/core/modules/book/book.module
@@ -468,7 +468,9 @@ function template_preprocess_book_export_html(&$variables) {
-  $variables['head'] = drupal_get_html_head();
+  // @todo: Commenting out the following line is wrong.
+  // @see https://www.drupal.org/node/2477223
+//  $variables['head'] = drupal_get_html_head();

should be "// @todo clean-up in URL" but as this not used anymore could be removed

+++ b/core/includes/common.inc
@@ -615,41 +526,17 @@ function drupal_merge_attached(array $a, array $b) {
 function drupal_process_attached(array $elements) {
   // Asset attachments are handled by \Drupal\Core\Asset\AssetResolver.
+  // Unsetting these parts preserved as a side-effect even though this function
+  // is deprecated.
   foreach (array('library', 'drupalSettings') as $type) {
     unset($elements['#attached'][$type]);
   }

that's actually makes no sense to keep,
it does nothing about from its doc-block

Mile23’s picture

Mile23’s picture

Status: Needs review » Needs work

The last submitted patch, 42: 2477223_42.patch, failed testing.

Mile23’s picture

Status: Needs work » Needs review
FileSize
30.15 KB
1011 bytes

Thanks for renaming everything, guys. :-)

xjm’s picture

Status: Needs review » Needs work

The beta evaluation for this issue is incorrect. Marking code for removal is not a prioritized change. (Removing code that was already deprecated as of beta 1 is prioritized, but that is very different.)

Also, API changes are not necessarily major issues. See the definition of the major priority.

My initial reaction is that I do not see the case for making this change during the beta, and so the actual benefit of doing it during the beta needs to be outlined in terms of the beta evaluation policy. Otherwise, the issue should be postponed. Thanks!

Mile23’s picture

Assigned: Unassigned » Mile23

The beta evaluation for this issue is incorrect. Marking code for removal is not a prioritized change. (Removing code that was already deprecated as of beta 1 is prioritized, but that is very different.)

My initial reaction is that I do not see the case for making this change during the beta, and so the actual benefit of doing it during the beta needs to be outlined in terms of the beta evaluation policy. Otherwise, the issue should be postponed. Thanks!

The change in *ths* issue is to remove usages of functions marked @deprecated before 8.0.0, which is why the beta evaluation is (mostly) correct.

The other issue is to address the question of whether drupal_process_attached() itself should be deprecated/removed. #2552865: Deprecate drupal_process_attached() for 8.0.0 and either throw an Exception or make it work via the RenderContext

Once that is settled (and it seems to be, given IRC chat), then we can figure out how to support two APIs, one of which no one wants.

Mile23’s picture

Assigned: Mile23 » Unassigned
Status: Needs work » Needs review
FileSize
34.64 KB
8.99 KB

Though it pains me in a way I can't quite put into words, I've grafted a static property on to HtmlResponseAttachmentsProcessor, called $legacyAttached.

drupal_process_attached() merely merges the given render array into this static.

Later in the process, HtmlResponseAttachmentsProcessor merges this static into the given attachments from the HtmlResponse.

Added tests to verify this behavior in HtmlResponseAttachmentsTest.

@deprecated tags and language will be decided in #2552865: Deprecate drupal_process_attached() for 8.0.0 and either throw an Exception or make it work via the RenderContext, so that's removed here.

BookTest hung on my computer so I have no idea whether it will pass or fail. Regardless, I updated the modification to book.module to point to #1338858: Include theme print stylesheets in Book printer friendly export This issue might be postponed on that one.

Mile23’s picture

Crell’s picture

xjm: I have to disagree with you. Many people have been working to replace these functions for months, if not a year. They offer no value anymore whatsoever, other than a year ago there was no better option. The better options have existed now for a while, and people should be using them instead. I think it highly unlikely that any existing contribs would be using them at this point, and even if one or two do it's an existing bug in those modules anyway. Retaining them serves no purpose other than to confuse people about what they should be doing and why there's a random global lying around.

Retaining these functions sound like a bad case of process over pragmatism. Other than procedural nitpicks is there any reason at all to keep them? If not, let pragmatism and code quality win and let's move forward.

Mile23’s picture

In IRC @mixologic (sorry if I got that name wrong) mentioned that google analytics module uses drupal_process_attached(), which it does in google_analytics_preprocess_item_list().

That does, in fact, make a good use case to explore the question of how to wrangle this beast to allow someone to attach javascript conditionally for a page item. Since it's a hook and only has \Drupal to go on, it accesses a global (which still exists at this late stage), called $pager_total_items from pager.module, to hijack and use for its own purposes. It's only functional as a side effect of other things working. That is, it's terrible.

I'd say it's code that deserves to break, but it's maintained by people who don't deserve to have to fix it.

The solution for this would be to create another event subscriber which happens before HtmlResponseAttachmentsTest and poke various 'html_head' needs into that, and then process them similar to the way HtmlResponseAttachmentsTest works. Since that's the case let's just leave drupal_process_attached() and not make them do it.

Much as I want to.

catch’s picture

Preprocess can use $variables['#attached'].

Mile23’s picture

OK, so regardless of the API change stuff, this issue is about removing deprecated functions which were marked as deprecated.

It does that.

How about a code review? :-)

catch’s picture

Status: Needs review » Needs work
+++ b/core/includes/common.inc
@@ -620,37 +532,20 @@ function drupal_merge_attached(array $a, array $b) {
+      HtmlResponseAttachmentsProcessor::$legacyAttached = $elements['#attached'];

Pretty sure that neither the static in HEAD nor this static class property is going to work for calls that get render cached.

This would need to use a render context similar to the way early calls to drupal_render() do.

Wim Leers’s picture

I think all that staticness — which AFAICT was added just to preserve some of the behavior of drupal_process_attached() — is highly problematic. And in fact unnecessary, because drupal_process_attached() should be removed, see #2552865-26: Deprecate drupal_process_attached() for 8.0.0 and either throw an Exception or make it work via the RenderContext.

As the current patch exquisitely demonstrates, drupal_process_attached() is highly problematic because it relies on a static. This is incompatible with render caching (as catch says in #53. Which is why the only callers in core are ancient tests (that likely don't even need it anymore), and HtmlResponseAttachmentsProcessor, which was specifically designed to not depend on the staticness of drupal_process_attached().

Many hundreds of hours have been spent on removing all the staticness with regards to asset/attachment handling in D8 — see https://www.drupal.org/node/2169605. drupal_process_attached() is the last remaining bit, and indeed was not marked as deprecated, but should've been, and has only been calling deprecated functions for a very long time now, which means it implicitly was marked deprecated.
See #2552865-26: Deprecate drupal_process_attached() for 8.0.0 and either throw an Exception or make it work via the RenderContext for details.

Mile23’s picture

It would be super-awesome if someone could add a test to demonstrate the cache fail.

'Needs tests' tag is still there. :-)

Mile23’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
36.13 KB
3.91 KB

So here's a test which *doesn't* demonstrate cache fails. It looks for X-Drupal-Cache: HIT.

@catch, @wim-leers: If there's a better way to demonstrate a render pipeline cache fail for this process, please add to the test. Thanks.

Mile23’s picture

... Sure would be nice if someone reviewed this.

joelpittet’s picture

Status: Needs review » Needs work

Here's a review:

  1. +++ b/core/lib/Drupal/Core/Render/HtmlResponseAttachmentsProcessor.php
    @@ -24,6 +35,16 @@
    +   * @var array
    +   * @todo Remove this in Drupal 9.
    

    Can this get a @deprecated instead of @todo?

  2. +++ b/core/lib/Drupal/Core/Render/HtmlResponseAttachmentsProcessor.php
    @@ -269,8 +344,103 @@ protected function setHeaders(HtmlResponse $response, array $headers) {
    +        // @todo: Change this to Html::escape() after https://www.drupal.org/node/2550945
    +        $href = '<' . SafeMarkup::checkPlain($attributes['href']) . '>;';
    

    This is landed so can be resolved.

  3. +++ b/core/modules/book/book.module
    @@ -468,7 +468,9 @@ function template_preprocess_book_export_html(&$variables) {
    -  $variables['head'] = drupal_get_html_head();
    +  // @todo: Commenting out the following line is wrong.
    +  // @see https://www.drupal.org/node/1338858
    +  // $variables['head'] = drupal_get_html_head();
    

    Any idea why it's wrong or how to resolve this @todo?

  4. +++ b/core/modules/system/src/Tests/Render/HtmlResponseAttachmentsTest.php
    @@ -0,0 +1,94 @@
    +    // @todo: Improve this test.
    +    $this->drupalGet('/render_attached_test/feed');
    

    How does this test need to be improved?

Mile23’s picture

Status: Needs work » Needs review
FileSize
35.75 KB
4.92 KB

Thanks.

#58.1: We're not sure whether it's deprecated, so there really shouldn't be a @todo, either. #2552865: Deprecate drupal_process_attached() for 8.0.0 and either throw an Exception or make it work via the RenderContext

#58.2: Done.

#58.3: It's really a bug in book, but we can fix it here, too, a little bit. The other issue: #1338858: Include theme print stylesheets in Book printer friendly export

#58.4: The test only did repeated xpath queries to find the feed ref. Improved to look for the specific test URL and then examine the attributes of that tag.

Mile23’s picture

Needed a reroll.

Fabianx’s picture

Status: Needs review » Needs work
+++ b/core/includes/common.inc
@@ -629,37 +541,20 @@ function drupal_merge_attached(array $a, array $b) {
+    if (empty(HtmlResponseAttachmentsProcessor::$legacyAttached)) {
+      HtmlResponseAttachmentsProcessor::$legacyAttached = $elements['#attached'];
+    }
+    else {
+      HtmlResponseAttachmentsProcessor::$legacyAttached =
+        BubbleableMetadata::mergeAttachments(
+          HtmlResponseAttachmentsProcessor::$legacyAttached,
+          $elements['#attached']
+        );

This approach is not compatible with SmartCache or Render Caching; better would be to just render #attached on the Renderer service to put it to where-ever it belongs.

e.g.:

  $build['#attached'] = $elements['#attached'];
  \Drupal::service('renderer')->render($build);

that keeps BC the closest while not hindering render caching, but the decision for that will be made in #2552865: Deprecate drupal_process_attached() for 8.0.0 and either throw an Exception or make it work via the RenderContext.

Mile23’s picture

This approach is not compatible with SmartCache or Render Caching

Please provide a test which illustrates this. See #54, #56.

That way we can make a solid argument for deprecation before 8.0.0.

Thanks.

Fabianx’s picture

I don't have time to do that, but a simple test is to just create a cacheable block, call drupal_process_attached() in the build method, place it in a region, then call the url twice.

Then assert both times the added html_head or whatever.

The first time will be there, the second it won't.

Wim Leers’s picture

Exactly what #63 says.

Many thanks for keeping this going, @Mile23, I promise faster reviews from now on.

Mile23’s picture

Status: Needs work » Needs review
FileSize
37.97 KB
3.21 KB

OK, this is very strange.

The new test fails when I do it manually (add the block, look at the front page, verify X-DPA-Cache-Test header, reload, verify header is gone).

But simpletest happily passes the test when run from the UI. I wonder if it's not re-initializing http headers between requests.

joelpittet’s picture

@Mile23 question:

+++ b/core/modules/system/tests/modules/render_attached_test/src/Plugin/Block/DrupalProcessAttachedBlock.php
@@ -0,0 +1,42 @@
+    \drupal_process_attached(['#attached' => ['http_header' => [['X-DPA-Cache-Test', 'HIT']]]]);

This shouldn't need a \ in the front of the function name does it?

Mile23’s picture

OK, a few things going on here:

1) The tests are vastly improved.

2) There are two patches here: One using the static introduced in a previous patch, and the other using the RenderContext system. Both pass, but I'm not sure which is better, since they both also pass #3.

3) I still have the situation where if I do the block test manually it fails, but passes in the UI and I'm guessing the testbot. Any help on how to reconcile these two things would be appreciated.

4) One interdiff tells you how to get from #65 to the static, the other tells you how to get from static to RenderContext.

The last submitted patch, 67: 2477223_67_rendercontext.patch, failed testing.

hass’s picture

@Mile23: You asked for an example in #2552865: Deprecate drupal_process_attached() for 8.0.0 and either throw an Exception or make it work via the RenderContext, but I do not understand why you are trying to fix a deprecated drupal_process_attached function?

Here is the example code:

function mymodule_preprocess_page_attachments(&$variables) {
    $variables['#attached']['html_head'][] = [
      [
        '#tag' => 'script',
        '#value' => 'test = 2;',
      ],
      'test2_script'
    ];
}

function mymodule_preprocess_item_list__search_results(&$variables) {
    $variables['#attached']['html_head'][] = [
      [
        '#tag' => 'script',
        '#value' => 'test = 1;',
        // If you remove the weight or set it to "0" the js inline code "test = 1;" will not order properly before "test = 2;" in html header.
        '#weight' => -200,
      ],
      'test1_script'
    ];
}

And with drupal_process_attached() the order is correct from first sight, but per Wim, this was pure luck and if you need to order it below "test = 1" than you are out of luck:

function mymodule_preprocess_item_list__search_results(&$variables) {
    $page['#attached']['html_head'][] = [
      [
        '#tag' => 'script',
        '#value' => 'test = 1;',
        '#weight' => -200, // not supported in drupal_process_attached()
      ],
      'test1_script'
    ];
    drupal_process_attached($page)
}
Mile23’s picture

@hass: Thanks.

I do not understand why you are trying to fix a deprecated drupal_process_attached function

We're trying to successfully deprecate drupal_add_*() functions in this issue. drupal_process_attached() isn't marked as deprecated (yet), so we have to support it here.

There's still one last chance to modify HTML head to your liking and re-order the items within it: hook_html_head_alter(). However, it might be deprecated as well: #2555069: Remove invocation of hook_html_head_alter()

hass’s picture

I no longer need drupal_process_attached() in Google Analytics and I'm more than happy that the weight feature came back and $variables works now. This for sure solves a lot of issues. I just documented it as an example what type of issues exists if drupal_process_attached() is used with render arrays.

Mile23’s picture

So here's the bug fix for the RenderContext version for #65

This version has a problem: When running the test manually, it doesn't work AT ALL.

How to repro:

  • Apply this patch.
  • Move the render_attached_test module from core/modules/system/tests/modules to just modules.
  • Install it.
  • Visit the block assignment page and assign the DrupalProcessAttachedBlock to be in the content region.
  • Load a page.
  • Look at the headers. You should see a status code of 418 (I'm a teapot), and X-Teapot-* headers, but you see none.

And yet, the test will probably pass on the testbot.

Someone smarter than I will have to figure it out.

Status: Needs review » Needs work

The last submitted patch, 72: 2477223_72_rendercontext.patch, failed testing.

Mile23’s picture

Status: Needs work » Needs review
FileSize
40.61 KB
1.8 KB

Bugfixes for the test to cleanly demonstrate that the RenderCache strategy for providing BC does not work.

For those who haven't memorized this issue, the RenderCache strategy looks like this:

function drupal_process_attached(array $elements) {
  $build['#attached'] = $elements['#attached'];
  \Drupal::service('renderer')->renderRoot($build);
}

The test creates a module with a controller which returns #attached render items.

The controller has two sets of routes: One returning a render array with #attached, and one calling drupal_process_attached() to add the attachments. It then compares the cache status for both over two page loads, showing whether we've cached the page.

The second test is to implement a block as an annotated plugin. This block instantiates the controller in order to grab its content and use drupal_process_attached() to handle the attachments. It then loads and reloads a page with that block, in order to find out whether the block's head/header/status attachments are cached properly.

As it turns out: FAIL. :-)

This leaves us with the static strategy to emulate the way drupal_add_*() worked before. As you can see in #67 this passes tests, but there is some ambiguity about running the tests locally.

joelpittet’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/system/src/Tests/Render/HtmlResponseAttachmentsTest.php
    @@ -61,6 +61,7 @@ public function testAttachments() {
    +    debug('Now using drupal_process_attached().');
    

    Left your tools on the floor.

  2. +++ b/core/modules/system/src/Tests/Render/HtmlResponseAttachmentsTest.php
    @@ -158,8 +164,13 @@ protected function assertHead() {
    -    $this->assertEqual($test_meta_attributes['test-attribute'], 'testy');
    ...
    +      $this->assertEqual($test_meta_attributes['test-attribute'], 'testy');
    

    This is an unfortunate name for an attribute value 'testy'. Maybe go for imaginative or friendly at least;)

The new tests look like an improvement to ensure feed link and meta data aren't missing, nice change.

Mile23’s picture

Left your tools on the floor.

Well, no. :-)

Also, in an ideal world, none of this code will be used because drupal_process_attached() will be deprecated.

The last submitted patch, 74: 2477223_74_rendercontext.patch, failed testing.

The last submitted patch, 74: 2477223_74_rendercontext.patch, failed testing.

Fabianx’s picture

function drupal_process_attached(array $elements) {
  $build['#attached'] = $elements['#attached'];
  \Drupal::service('renderer')->renderRoot($build);
}

This can't work!

It must use ->render().

renderRoot() throws away any context data, which is especially not what we want.

And if anything gives back an Exception() of missing render context than its usage of drupal_process_attached() needs to be fixed.

As said before the static caching strategy fails with blocks, which is already a bug in HEAD, hence this also not being critical.

We either go with ->render() plus trigger_error E_USER_DEPRECATED or remove drupal_process_attached() completely.

The static strategy just means that any other caching in upper levels will break and it will also now break with smart_cache :).

Mile23’s picture

Status: Needs work » Needs review
FileSize
40.56 KB
2.51 KB

This can't work!

It must use ->render().

renderRoot() throws away any context data, which is especially not what we want.

@fabianx: thanks. I was trying to ping you and @wimleers in IRC about it, but no joy. ->render() was throwing an exception telling me to use renderRoot(), which didn't throw an exception.

I just tried it now and it doesn't throw an exception any longer.

Also, there should be an issue about documenting the various render() versions, because, just like in Drupal 7, they all overlap and it's completely unclear which you should use, unless you already know, which I don't.

Also addressed #75.

Status: Needs review » Needs work

The last submitted patch, 80: 2477223_80.patch, failed testing.

Mile23’s picture

OK, it doesn't throw that exception locally, but does on the testbot.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
41.21 KB
747 bytes

Great work here, @Mile23! Sorry for not having gotten back to you earlier :(

It should throw that exception if you run the tests using run-tests.sh. The culprit is \Drupal\simpletest\KernelTestBase::render():

  protected function render(array &$elements) {
    $content = $this->container->get('renderer')->renderRoot($elements);
    drupal_process_attached($elements);
    $this->setRawContent($content);
    $this->verbose('<pre style="white-space: pre-wrap">' . Html::escape($content));
    return $content;
  }

That drupal_process_attached() call is running outside of a render context! That's also what the exception is complaining about. So we need to fix that.

… and in fact that drupal_process_attached() call is completely obsolete. So we can just delete it, and we'll be fine :)

Mile23’s picture

Thanks.

Ossum. So now we have BC for drupal_process_attached().

Someone RTBC this, quick, so we can then argue about whether to demolish all this work by deprecating for 8 or 9 in #2552865: Deprecate drupal_process_attached() for 8.0.0 and either throw an Exception or make it work via the RenderContext

The last submitted patch, 80: 2477223_80.patch, failed testing.

Mile23’s picture

Updating stuff. Also added follow-up #2566619: Deprecate drupal_http_header_attributes()

Also, since it's backward-compatible, there is no need for a change record here. There will be a need for one in #2552865: Deprecate drupal_process_attached() for 8.0.0 and either throw an Exception or make it work via the RenderContext

andypost’s picture

It looks good to go except "nit" and a way for contrib to provide own attachment types

  1. +++ b/core/lib/Drupal/Core/Render/AttachmentsResponseProcessorInterface.php
    @@ -17,13 +19,38 @@
    +   *   Drupal\Core\Render\HtmlResponse.
    

    needs \ to full name

  2. +++ b/core/lib/Drupal/Core/Render/HtmlResponseAttachmentsProcessor.php
    @@ -117,27 +139,67 @@ public function processAttachments(AttachmentsInterface $response) {
    +    $unsupported_types = array_diff(
    +      array_keys($attached),
    +      ['html_head', 'feed', 'html_head_link', 'http_header', 'library', 'html_response_attachment_placeholders', 'placeholders', 'drupalSettings']
    

    I think we need to keep it in Settings or as kernel argument

    Otoh having that array as class static is good too, then think about to add ability to easily add any contrib attach types...
    maybe contrib can inherit then class needs clean-up

+++ b/core/lib/Drupal/Core/Render/HtmlResponseAttachmentsProcessor.php
@@ -117,27 +139,67 @@ public function processAttachments(AttachmentsInterface $response) {
+    if (!empty($unsupported_types)) {
+      throw new \LogicException(sprintf('You are not allowed to use %s in #attached.', implode(', ', $unsupported_types)));

that mostly about ability to process this kind of attachments

Mile23’s picture

Thanks for the review.

Otoh having that array as class static is good too, then think about to add ability to easily add any contrib attach types...

That's a feature request outside the scope of this issue, which deals with deprecating some functions.

needs \ to full name

Nice catch.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
40.71 KB
1.95 KB

Someone RTBC this, quick, so we can then argue about whether to demolish all this work by deprecating for 8 or 9

That would not be demolishing all the work in this issue; quite a bit in this patch is not only about drupal_process_attached(). As the issue title indicates, it's also about refactoring all the HTML head stuff into the attachments response processor :)

Mile23++


  1. +++ b/core/includes/common.inc
    @@ -24,6 +24,7 @@
    +use Drupal\Core\Render\HtmlResponseAttachmentsProcessor;
    

    Can be removed.

  2. +++ b/core/lib/Drupal/Core/Render/AttachmentsResponseProcessorInterface.php
    @@ -17,13 +19,38 @@
    +   *   Thrown when the $response parameter is not a
    +   *   \Drupal\Core\Render\HtmlResponse.
    

    This is wrong; this is the generic interface.

    Each implementation of this interface will complain if it's not given the specific type of response it cares about.

  3. +++ b/core/modules/system/src/Tests/Render/HtmlResponseAttachmentsTest.php
    @@ -0,0 +1,175 @@
    +  public function testSmartCache() {
    

    This doesn't test Smart Cache (which ended up getting a different name BTW). It just tests render caching.

    So I think testRenderCachedBlock() would be better.

Since all of the above really are simple nits, and the rest is IMO ready, fixing those nits myself and RTBC'ing, so that this can finally get committed.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 89: 2477223_89.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community

DrupalCI: green.
PIFR: fail, stupid testbot: GET http://ec2-52-27-255-15.us-west-2.compute.amazonaws.com/checkout/user/login returned 0 (0 bytes). .

This is green, just testbot being utterly broken.

Mile23’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
40.95 KB
623 bytes

+1 on all those changes, except:

+++ b/core/lib/Drupal/Core/Render/AttachmentsResponseProcessorInterface.php
@@ -17,13 +19,38 @@
+ * Thrown when the $response parameter is not a
+ * \Drupal\Core\Render\HtmlResponse.
This is wrong; this is the generic interface.

Each implementation of this interface will complain if it's not given the specific type of response it cares about.

We really need to explain that in the docblock.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Works for me!

Fabianx’s picture

RTBC +1, while having that list of attachment types be extensible is a little problematic for contrib (and core) right now, it does not belong in this issue as it is just a refactoring.

Also making that list extensible is only a very non-disruptive API addition and no behavior change, hence this can indeed be follow-up.

Mile23’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
40.31 KB
Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Still looks good.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 95: 2477223_95.patch, failed testing.

The last submitted patch, 95: 2477223_95.patch, failed testing.

catch’s picture

Fail looks real:

Invalid #attachment 'drupal_process_states' allowed
Mile23’s picture

Status: Needs work » Needs review
FileSize
40.46 KB
720 bytes

I can't repro those fails.

Here's the more explicit test from before.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

#100 makes sense: ->render() doesn't process attachments, HtmlResponseAttachmentsProcessor does.

That's also what #92 had, but was lost in the rebase. I earlier missed the fact that this was lost in that rebase.

Back to RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 100: 2477223_100.patch, failed testing.

Mile23’s picture

Again, I can't repro the failing test, which is \Drupal\field\Tests\Migrate\d7\MigrateViewModesTest.

Possibly unrelated.

joelpittet queued 100: 2477223_100.patch for re-testing.

The last submitted patch, 100: 2477223_100.patch, failed testing.

Mile23’s picture

Speaking of which.... We'll need a re-roll after #2568511: Fix broken test: KernelTestBase::render

Wim Leers queued 100: 2477223_100.patch for re-testing.

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community

Green again. Testbot--

hass’s picture

Wim Leers’s picture

#109: I replied there.

catch’s picture

Issue tags: +rc deadline

Tagging RC deadline. We might be able to do some of this in a minor release, but I don't think it's doable in a patch release.

Mile23’s picture

It's ready to go now, has backwards-compatibility, deprecates code to be removed before 8.0.0.

What's the hanging question?

catch’s picture

For a start I'm not sure if we should add the backwards compatibility at all reading the patch it's a two-line bc layer so meh. Also it takes more time to review a 40kb patch than it does to tag it so it doesn't get forgotten.

ianthomas_uk’s picture

+++ b/core/lib/Drupal/Core/Render/HtmlResponseAttachmentsProcessor.php
@@ -117,27 +139,67 @@ public function processAttachments(AttachmentsInterface $response) {
+        // Invoke hook_html_head_alter().
+        $this->moduleHandler->alter('html_head', $html_head);

As discussed in #2555069: Remove invocation of hook_html_head_alter(), hook_html_head_alter() is a subset of hook_page_attachments_alter() and is therefore unnecessary.

It is already an undocumented hook.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 100: 2477223_100.patch, failed testing.

ianthomas_uk’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
26.14 KB
Mile23’s picture

fail: [Completion check] Line 56 of core/modules/field/src/Tests/Migrate/d7/MigrateViewModesTest.php:
The test did not complete due to a fatal error.

Boo, migrate!

Re #114: hook_html_head_alter() needs a change record so it has to stay for now. It's an easy reroll either direction.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks!

  • catch committed b544422 on 8.0.x
    Issue #2477223 by Mile23, Wim Leers, j2r, ianthomas_uk, jaimeguzman,...

Status: Fixed » Needs work

The last submitted patch, 116: 2477223_116.patch, failed testing.

Mile23’s picture

Is it really fixed? Or is that the testbot burping?

ianthomas_uk’s picture

Status: Needs work » Fixed

It's fixed. Looks like the testbot failed to apply my patch, because by the time it started the test, the patch had been applied to head.

Mile23’s picture

Mile23’s picture

Status: Fixed » Closed (fixed)

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