Problem/Motivation

$build['#attached']['drupal_add_feed']
This is a) a confusing API and b) we don't want to keep drupal_add_feed forever.

Proposed resolution

  • Rename all drupal_add_* methods to use an underscore (like css/js)
  • Make it possible to use $build['#attached']['feed|html_head|html_head_link']

Remaining tasks

User interface changes

API changes

  • '#attached][drupal_add_feed' => 'feed'
  • '#attached][drupal_html_head' => 'html_head'
  • '#attached][drupal_add_http_header' => 'http_head'

drupal_add_region_content() and drupal_set_region_content() are also removed - these were dead code already.

Original report by @username

We removed direct calls to drupal_add_html_head_link() but didn't deprecate either that or drupal_add_feed().

Opening this as a critical follow-up, we realised this during the DrupalCon Amsterdam discussion about page rendering.

https://api.drupal.org/api/drupal/core%21includes%21common.inc/function/...

https://api.drupal.org/api/drupal/core%21includes%21common.inc/function/...

Additionally, we decided to formalize the #attached support in https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Page%21Re... so it doesn't use the function names (i.e. you can use #attached['head_link'] or similar). Looks like dynamic function support has already been removed there.

Files: 
CommentFileSizeAuthor
#26 2350877-25.patch26.88 KBhussainweb
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,046 pass(es). View
#26 interdiff-21-26.txt465 byteshussainweb
#21 2350877-21.patch26.88 KBWim Leers
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,047 pass(es), 4 fail(s), and 0 exception(s). View
#9 interdiff.txt5.41 KBdawehner
#6 interdiff.txt20.21 KBdawehner
#6 2350877-6.patch24.69 KBdawehner
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,014 pass(es). View
#1 2350877-1.patch5.81 KBdawehner
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,995 pass(es). View

Comments

dawehner’s picture

FileSize
5.81 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,995 pass(es). View

I got distracted quite a bit, so now this also removes drupal_region_add_content() which does not work anymore anyway.

dawehner’s picture

Status: Active » Needs review
Issue tags: -API change, -Needs issue summary update +undefined

There is also a tag for it

dawehner’s picture

Issue tags: -undefined +page rendering

nice tag!

catch’s picture

Wim Leers’s picture

Title: Deprecate/rename drupal_add_feed()/drupal_add_html_head_link() » Deprecate/rename drupal_add_feed()/drupal_add_html_head()/drupal_add_html_head_link(), allow to be set declaratively in #attached
dawehner’s picture

FileSize
24.69 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,014 pass(es). View
20.21 KB
  • Renamed a couple of methods
  • Introduced 'feed', 'html_head', 'html_head_link' and 'http_header'
  • Invalidated non-matching attachments.
ianthomas_uk’s picture

Is this also the place to take account of HTTP Headers? #2307723: Provide a replacement for drupal_add_http_header

ianthomas_uk’s picture

Title: Deprecate/rename drupal_add_feed()/drupal_add_html_head()/drupal_add_html_head_link(), allow to be set declaratively in #attached » Deprecate/rename drupal_add_feed()/drupal_add_html_head()/drupal_add_html_head_link()/drupal_add_http_header(), allow to be set declaratively in #attached

Ah, I see you've already added support for ['#attached']['http_header'] in the patch, but we should also rename drupal_add_http_header() so people don't think they can call it directly.

dawehner’s picture

FileSize
5.41 KB
29.36 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,015 pass(es). View

So yeah let's do that.

dawehner’s picture

Issue summary: View changes

Update IS

dawehner’s picture

Issue summary: View changes
FileSize
1.38 KB
29.19 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,060 pass(es). View

@catch asked to drop the support for it.
Found one documentation instance.

catch’s picture

Status: Needs review » Reviewed & tested by the community

Looks great now!

alexpott’s picture

Issue tags: +Needs change record

We're going to need a CR for this.

alexpott’s picture

Title: Deprecate/rename drupal_add_feed()/drupal_add_html_head()/drupal_add_html_head_link()/drupal_add_http_header(), allow to be set declaratively in #attached » Deprecate/rename drupal_add_feed(), drupal_add_html_head(), drupal_add_html_head_link(), drupal_add_http_header(), and allow to be set declaratively in #attached
catch’s picture

https://www.drupal.org/node/2160069 was completely out of date, only dealt with one function, and still used url(). Since we haven't sorted out the url API yet, I left stub examples in there, but covered the other functions.

catch’s picture

Issue summary: View changes
alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/lib/Drupal/Core/Page/RenderHtmlRenderer.php
@@ -52,23 +52,32 @@ public function render(array $render_array) {
+      '_drupal_add_feed' => [],
...
+      '_drupal_add_html_head' => [],
...
+      '_drupal_add_html_head_link' => [],
...
+    foreach ($attached['_drupal_add_feed'] as $feed) {
       $fragment->addLinkElement(new FeedLinkElement($feed[1], $this->urlGenerator->generateFromPath($feed[0])));
     }
...
+    foreach ($attached['_drupal_add_html_head_link'] as $link) {
+      $fragment->addLinkElement(new LinkElement($this->urlGenerator->generateFromPath($link[0]['href']), $link[0]['rel']));
+    }

Looks unnecessary.

hussainweb’s picture

Status: Needs work » Needs review
FileSize
1.44 KB
26.8 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,040 pass(es). View

I am attaching a reroll, plus changes as per #17. The interdiff is only for changes in that comment, not for the reroll.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

+1

Wim Leers’s picture

FileSize
26.71 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,040 pass(es), 4 fail(s), and 0 exception(s). View
1.67 KB

Fixed the only remaining todo and the following nitpicks:

  1. +++ b/core/modules/rdf/rdf.module
    @@ -367,7 +367,9 @@ function rdf_preprocess_user(&$variables) {
    +      // @todo Convert once https://www.drupal.org/node/2346369 is in.
    +      $build['#attached']['html_head'][] = [$username_meta, 'rdf_user_username'];
    +      drupal_render($build);
    

    This already is in now :)

  2. +++ b/core/modules/system/src/Tests/Common/RenderTest.php
    @@ -1181,4 +1181,20 @@ protected function randomContextValue() {
    +    // Specify an invalid render array.
    

    "invalid attachments in a render array"

  3. +++ b/core/modules/system/src/Tests/Common/RenderTest.php
    @@ -1181,4 +1181,20 @@ protected function randomContextValue() {
    +      $this->fail("Invalid #attachmend 'drupal_process_states' allowed");
    ...
    +      $this->pass("Invalid #attachmend 'drupal_process_states' not allowed");
    

    s/attachmend/attachment/ :)

  4. +++ b/core/modules/views/src/Plugin/views/style/Rss.php
    @@ -56,8 +56,8 @@ public function attachTo(array &$build, $display_id, $path, $title) {
    -      // Add a call for drupal_add_html_head_link to the view attached data.
    ...
    +      // Add the RSS icon to the view.
    

    That's not really what's happening here. This associates a link with an alternative representation, not the RSS feed icon :)

Wim Leers’s picture

FileSize
26.88 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,047 pass(es), 4 fail(s), and 0 exception(s). View
1001 bytes

I forgot to fix my own 4th point. Stupid!

Wim Leers’s picture

Issue tags: -Needs change record

The existing change record https://www.drupal.org/node/2160069 has already been updated.

catch’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/modules/rdf/rdf.module
@@ -367,7 +367,7 @@ function rdf_preprocess_user(&$variables) {
+      $build['#attached']['html_head'][] = [$username_meta, 'rdf_user_username'];

s/build/variables/ ?

The last submitted patch, 20: 2350877-20.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 21: 2350877-21.patch, failed testing.

hussainweb’s picture

Status: Needs work » Needs review
FileSize
465 bytes
26.88 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,046 pass(es). View

I hope this fixes some of the failures.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

My patch was flawed, as #23 shows. And that's precisely what fixed the test failures also. Hurray :)

catch’s picture

Status: Reviewed & tested by the community » Fixed

I opened, reviewed and RTBCed this issue. But since Alex has reviewed it too now and that feedback has been dealt with, I think I can commit it as well without too much guilt.

Committed/pushed to 8.0.x, thanks!

  • catch committed 85145ba on 8.0.x
    Issue #2350877 by dawehner, Wim Leers, hussainweb: Deprecate/rename...
tstoeckler’s picture

Category: Task » Bug report
Status: Fixed » Active

Unless I am majorly missing something this patch is a huge regression for contrib due to

Invalidated non-matching attachments.

In Drupal 7 due to the call_user_func() call, it was absolutely possible for contrib modules to participate in the #attached process. You can - for example - attach external libraries to render arrays by using '#attached' => ['libraries_load' => [['foo']]]. I don't see how we could achieve the same with this patch.

Edit: That's in D7 and with Libraries API.

Unless someone can show me how to achieve the same, this patch should either be rolled back or amended. I can't really tell whether the latter would be feasible or not.

ianthomas_uk’s picture

Do we want to allow contrib to do that? I think the libraries example would now be covered by core, but are there other use cases? I can see us wanting to further refactor the code that handles #attached, which might be impossible if we keep support for arbitrary #attached functions.

If we do, we could fairly easily support that in drupal_process_attached by restoring call_user_func_array instead of throwing an exception. I'd still keep the rest of the switch statement so we can have the friendly ['#attached']['feed'] style syntax. Is it worth making modules register the functions that can be called via #attached, so people don't just attach any old function?

catch’s picture

I can see us wanting to further refactor the code that handles #attached, which might be impossible if we keep support for arbitrary #attached functions.

Yes that was part of the discussion where we said we should remove support.

Right now the _drupal_add_*() functions that get called in the end are horrible legacy static things that we want to refactor still.

IMO adding support for more methods is either of the following:

1. Core issues for anything missing (i.e. the 7.x use cases that should be handled in 8.x already now)

2. New major issue to expand support for contrib - but needs a defined use case to do so.

We can make API additions at any point during the 8.x cycle, but it's much less easy to remove support for side effects like the call_user_func() on any string call.

Leaving this open but if someone fancies opening either #1 and/or #2 that feels like the right way forward.

dawehner’s picture

In Drupal 7 due to the call_user_func() call, it was absolutely possible for contrib modules to participate in the #attached process. You can - for example - attach external libraries to render arrays by using '#attached' => ['libraries_load' => [['foo']]]. I don't see how we could achieve the same with this patch.

It sounds a bit like we need some basic registry of key -> callable for those kind of bit. Do you know of any other usecase in core?

xjm’s picture

Issue tags: +D8 cacheability
tstoeckler’s picture

I think the libraries example would now be covered by core, but are there other use cases?

Can you give some examples of this? I'm pretty certain that this is not the case, to be honest.

I can see us wanting to further refactor the code that handles #attached, which might be impossible if we keep support for arbitrary #attached functions.

Yes that was part of the discussion where we said we should remove support.

Sadly the results of this discussion did not make it into the issue summary. The quote I gave in #30 is the only reference of this.

(i.e. the 7.x use cases that should be handled in 8.x already now)

Again, I fail to see how this is handled by 8.0.x.

2. New major issue to expand support for contrib - but needs a defined use case to do so.

The alternative to having Libraries API use #attached in some way, is to use a static libraries_load() call in Drupal 8 which is not something I would like to encourage.

I think what would be sufficient already would be a mechanism for contrib to to have a mechanism (basically just some callable) which can process a more domain-specific structure into #attached][js and #attached][css. That would totally serve Libaries API's use-case.

For example:

// Drupal 7
$build['#attached']['libraries_load'][] = ['foo'];

function libraries_load($name) {
  drupal_add_js();
  drupal_add_css();
}

// Drupal 8
// Not sure about the exact DX yet, this is just an example.
$build['#attached']['libraries_load'][] = ['foo'];

function libraries_load(&$element, $name) {
  $element['#attached']['js'][];
  $element['#attached']['css'][];
}

Not sure if that would make the implementation easier.

rjacobs’s picture

@tstoeckler, it's good to see that you are already bringing the Libraries API case into the discussion, as that may be the most salient example from contrib of jarring ramifications (e.g. one of the D8 projects I'm working on now was already affected as a result of this change coupled with the dependency on Librareis API).

I think something similar to #35 (an intermediary function to alter the build array directly) also came up toward the end of an old Libraries API thread: #2183087: Find a way around _drupal_add_js/css. Could some part of that actually be handled by core, or would the onus shift to each contrib module (like Libraries API) to provide their own unique methods to append-to $build arrays? Maybe as catch noted that has to be a follow-up issue?

tstoeckler’s picture

@rjacobs I don't see how something like this could be done in contrib, TBH. If there is a way, I'd gladly implement it in Libraries API. And the Libraries API code does not even have to be elegant. I just care for the DX of people using Libraries API. I.e. we could circumvent this problem by forcing people to add a #pre_render callback provided by Libraries API which would take care of the processing but I don't find that acceptable DX.

I personally would find it backwards to open a follow-up for resolving a regression, but I find a lot of things about our issue queue backwards, so as long as we find a solution, I'm happy.

rjacobs’s picture

Yea, it looks like preserving the current DX for important cases like Libraries API would be impossible without additional core changes. My question about what contrib could do in reaction was more akin to your #pre_render callback example, which I do agree is less than ideal (at least from my perspective as a contrib maintainer).

One option would be to add-back the call_user_func_array($callback, $args); option to drupal_process_attached() for the default case (in the switch). However, this would propagate the dependence on _drupal_add_* functions in the callbacks. Barring that, it looks like modules just need an opportunity to interact and alter $elements['#attached'] if certain "unknown" $callback strings are found. If this was an option I guess the related logic would have to fire quite early in drupal_process_attached() (in case module's added new js, css or library entries).

dawehner’s picture

One idea tobias and I had after a short conversation: Make the available things in #attached tagged services. With this contrib
modules could extend the list of available things in there without having the requirement to expose every callable.

Does someone has any opinions about that?

Berdir’s picture

Sounds interesting. Or just fixed service names? Like attached_processor.name_of_attached_key? We don't really need to have a list of them? And we need the container anyway to lazy-load those services. The pattern is maybe not too common, but I think people are kind of familar with specially-named services, like cache bins, even though that is not a requirement there.

When porting to this, I also noticed that html_head still requires that super-weird structure so that it can call call_user_func_array() on the value. That is extremely un-intuitive. As it is hardcoded now, I'm wondering if we want to make the DX there better (would need another API change of course, unless we want to keep a BC layer). drupal_add_html_head has that second argument as a unique identifier, so that could be the array key for example. The same would work for tagged services, afaik we already use the key for something (js settings or something?) so we would have to pass the key and value to those services anyway.

So instead of

$element['#attached']['html_head'][] = array($head_thingy, 'some-unique-key');

You could write:

$element['#attached']['html_head']['some-unique-key'] = $head_thingy;
catch’s picture

Was thinking of something like tagged services at DrupalCon, I'd be fine with that. However what the functions currently do and what they should be doing is pretty far apart, we might want to wait for #2352155: Remove HtmlFragment/HtmlPage which I believe from discussion with Wim puts us in a position to get rid of the reliance on statics.

What exactly is it that libraries API does in 8.x that can't be done with core library support?

Berdir’s picture

libraries.module is about external libraries in the /libraries folder, it takes care of checking they existing and providing the correct actual path (it can be /libraries or /profiles/something/libraries for example). That is still needed for everything but core, which is in the special position of being allowed to just include external dependencies.

Some things that were done with libraries.module in 7.x can now be done with composer dependencies, but not every library out there is already a composer package and it doesn't help with assets yet.

One specific example where I am using it in 8.x is http://cgit.drupalcode.org/jw_player/tree/?h=8.x-1.x. The approach I chose there is to implement jw_player_libraries_info() for libraries.module and then based on that dynamically expand my "jw_player.libraries.yml" definition with jw_player_library_info_alter() (Yes, the hook names there are *that* confusing... :(). That then allows me to use #attached with library. As I didn't even occur to me that the callback thing is possible ;)

Wim Leers’s picture

First of all, AFAIK Libraries API is the only example of a module needing to have things #attached. Correct me if I'm wrong.


@tstoeckler: the answer below is intended to be objective, and criticizes the D7 code and proposes a better way forward. Please don't take it personal.

Make the available things in #attached tagged services

This is a possibility.

But that brings me to the point of: what processing would it then do? Libraries API has one purpose: allowing multiple modules to use the same asset library that is not available in Drupal core. But in Drupal 7, Libraries API is utterly broken in several ways:

  1. Most importantly, Libraries API does not expose libraries as… libraries! This means you cannot declare dependencies on Libraries API libraries! Puzzlingly, that is an open issue that barely got any attention: #1386368: Register Libraries in hook_library(). For now, e.g. navbar module works around this by having its own hacky conversion: http://cgit.drupalcode.org/navbar/tree/navbar.module?id=503dae836bc9d772.... But there's more…
  2. Reliance on statics, a la drupal_add_css(), even though #attached was indeed already a soft requirement in D7, to ensure all necessary assets are loaded in case of cache hits: libraries_load($name);
  3. Attaching is supported (but not recommended! :()… by means of the following, which still uses the static: $element['#attached']['libraries_load'][] = array('myAwesomeLibrary');
  4. Extremely painful detection and variant (unminified/non-minified) handling: undocumented, and apparently we were the first to do this in early 2014 even though that seems like the extremely common case: #2167021: Revamp Libraries API integration
  5. The key purpose of this module, having different modules being able to declare which libraries they want to use, does not work reliably: #2201125: Underscore library not loading: different library definitions overriding each other, #2334769: Incompatible with underscore module

(Libraries API developer docs are at https://www.drupal.org/node/1342238)

Problem 1 must be solved in Drupal 8, since Drupal 8 much more strictly requires dependencies to be declared. That's how Drupal 8 succeeds in loading only the necessary JS.

Therefore, I think the answer to all of this can be quite simple: Libraries API is in the business of not providing a libraries.info.yml file, since it doesn't come with a static list of available libraries. Instead, it is in the business of implementing hook_library_info_alter() and populating that with the libraries it provides. So that e.g. mymodule/some-library can declare a dependency on libraries/foo or libraries/react or libraries/angular or …
Then there is no need for $element['#attached']['libraries_load'] = … anymore, because the libraries Libraries API makes available will just be attachable like this: $element['#attached']['library'][] = 'libraries/foo';.

This would make Libraries API fit in perfectly with the existing architecture. Note that this is also how it could (and IMO should) work in Drupal 7.

If problem 1 is to be solved, problems 2 and 3 go away automatically. Problems 4 and 5 are the real problem space for Libraries API to solve, both in 7 and 8, and beyond.

Is my proposal flawed, because I'm overlooking something?


When porting to this, I also noticed that html_head still requires that super-weird structure so that it can call call_user_func_array() on the value. That is extremely un-intuitive.

Indeed, this is *extremely* bad DX. We should fix that. We can easily fix that. Very few people will be affected anyway.

Danny Englander’s picture

I am wondering if we can clarify the status of this issue? It's linked to in the release notes for drupal 8.0.0-beta2 but it does not looked fixed. Indeed, in my contib theme, I am now getting a Call to undefined function drupal_add_html_head() message which results in a WSOD. It had previously worked fine in drupal 8.0.0-beta1.

ianthomas_uk’s picture

RE #44, see the change request linked in the right bar on this issue, https://www.drupal.org/node/2160069

The issue was fixed, which involved removing some functionality. It is still open while we work out how to handle contrib modules that relied on that functionality (which doesn't apply in many cases).

catch’s picture

Status: Active » Fixed

Opened #2358727: Figure out what if anything libraries API needs for #attached support, I'm not at all clear what's actually missing there. Moving this back to fixed.

@Danny Englander: drupal_add_html_head() won't be coming back, the change notice covers the changes you'll need to make in your theme.

tstoeckler’s picture

Sorry for taking a while to respond here, will follow up in the other issue now.

karolus’s picture

I've been coming across the same issues that Danny Englander has in post #44, and have posted my code here. Have been testing things, but so far, the only successful way I have found to add a meta tag to the head was by hacking DefaultHtmlFragmentRenderer.php in core, which I know is a bad idea.

Danny Englander’s picture

@Karolus, I posted a question on Drupal Answers in regard to this. I've been trying to figure this out all morning but still no luck. I've even tried some of the examples from core using the new hook_page_attachments (drupal 8 dev) as well as hook_page_build / hook_page_alter in combo with the new suggested method but still no luck after drush cr.

karolus’s picture

@Danny Englander:
Yes, found your question there earlier--it was pretty much the same thing I was trying. Going into more detail onto how I hacked core to get it to work (know this isn't the right way to do things):

Went to core/modules/system/src/Tests/System/DefaultHtmlFragmentRenderer.php and added this line below line 157:
$page->addMetaElement(new MetaElement(NULL, array('http-equiv' => 'X-UA-Compatible', 'content' => 'IE=edge,chrome=1')));

In doing this, the meta tag worked as it should.

Danny Englander’s picture

@karolus - I added a working example to the change notice. However, there are now two viewport tags in my case, one from core and the one added from my theme so I am not sure how to handle that.

Status: Fixed » Closed (fixed)

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

delta’s picture

When I add this code in a hook_views_pre_render($view)

  $description = [
    '#tag' => 'title',
    '#value' => 'My awesome title',
  ];
  $view->element['#attached']['html_head'][] = [$description, 'title'];

I got two tags <title></title> in the <head>.

One with the views page title, and the one I added in my hook.. The one I added in the hook views pre render is in first position.

Any thought ?

joelpittet’s picture

@delta could you post a new issue and cross reference it here? This has been closed for some time so people aren't actively look for the issue you had.

delta’s picture

@joelpittet Thanks, it's done https://www.drupal.org/node/2665378