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

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

There is also a tag for it

nice tag!

• Renamed a couple of methods
• Introduced 'feed', 'html_head', 'html_head_link' and 'http_header'
• Invalidated non-matching attachments.

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

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.

So yeah let's do that.

Update IS

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

We're going to need a CR for this.

+++ 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.

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

+1

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 :)

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

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

I hope this fixes some of the failures.

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

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.

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?

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?

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.

@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?

@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.

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

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?

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;

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 ;)

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.

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.

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

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

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.

@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.

@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.

@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.

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 ?

@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.

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