Deprecate drupal_process_attached() for 8.0.0 and either throw an Exception or make it work via the RenderContext

+1 to remove

Wouldn't the right thing be to just remove it?

It hasn't had a deprecation phase, so this is weird.

+1 for deprecate and then quickly remove.

Suppose everyone agree

This seems like a "going through the motions" for nobody's benefit, considering we are almost at RC. It is like saying "we deprecated this a week ago and now you are upset we removed it today?". This reminds me of https://youtu.be/DDYWdABRQIo?t=50s

There are specific criteria for critical issues. See here: https://www.drupal.org/core/issue-priority#critical

This issue does not meet those criteria. Based on my assessment as a release manager, the correct priority for this issue is normal. Thanks!

Oh also, if we do decide to deprecate this, it should be deprecated for removal for 9.x instead. The release of beta 1 was the point at which we stopped deprecating things for removal in 8.x.

Discussed with @Mile23.

What actually needs to happen here is that the helpers actually need to be deprecated for 9.x at this point (and not removed), since this function was not marked deprecated and there is already D8 contrib code using it.

I definitely support the deprecation of drupal_process_attached for removal in 9.x, since it is too late to remove it for 8.x. The documentation needs to be clearer for people who are trying to upgrade their 7.x modules though, just "Specify attached resources and directives in the render array." doesn't really tell a developer new to 8.x what they need to do, so they'll probably end up just leaving the call to drupal_process_attached.

Should the docs be rolled into https://www.drupal.org/node/2169605 which can then be referenced from drupal_process_attached?

Oh also, if we do decide to deprecate this, it should be deprecated for removal for 9.x instead. The release of beta 1 was the point at which we stopped deprecating things for removal in 8.x.

That'd be very sad, because…

this function was not marked deprecated and there is already D8 contrib code using it.

… I don't think it's true that D8 contrib code is using it. If there is such code, I'd bet that it's because they were doing it in D7 and haven't upgraded it since.

We may not have marked drupal_process_attached() as deprecated, but that's because it's really an internal API. Since January 21, 2015 (#2368797: Optimize ajaxPageState to keep Drupal 8 sites fast on high-latency networks, prevent CSS/JS aggregation from taking down sites and use HTTP GET for AJAX requests), drupal_process_attached() already ignores ['#attached']['library'] and ['#attached']['drupalSettings']. Since a week ago (#2467759: Refactor drupal_process_attached() so it doesn't depend on drupal_add_http_header()), it also doesn't do anything for ['#attached']['http_header'] and once #2477223: Refactor _drupal_add_html_head, drupal_get_html_head, _drupal_add_html_head_link into the attachments processor, remove from common.inc. lands, drupal_process_attached() will be an empty shell.
The reason #2467759 and #2477223 are able to land at this time is because they remove already-deprecated functions. So, effectively, drupal_process_attached() has only been calling deprecated functions for a very long time now. We simply forgot to deprecate it also.

So, sure, we could keep it around, but it'd literally be a no-op. The only modules in contrib/custom that were calling this in D7 were those that had AJAX responses, which is a tiny fraction. In D8, they'd use AjaxResponse, which already handles #attached for them (also since January 21, 2015, but in a different issue: #2347469: Rendering forms in AjaxResponses does not attach assets automatically).

In conclusion: it really makes no sense to keep this. It is indeed very unfortunate that we forgot to deprecate this function. But at the same time, all of the things it calls were already deprecated, and it really was a kind of internal API anyway (as further evidenced by the calls in D7 core).

That's what I mean—if it is going to be useless, remove it.

1. +++ b/core/includes/common.inc
   @@ -613,17 +613,30 @@ function drupal_merge_attached(array $a, array $b) {
   + * @deprecated Will be removed before Drupal 8.0.0. Specify ['#attached']
   + *   elements in the render array from a controller. They will be rendered into
   

   It's self explanatory but should we put a little example in a @code block here (and in the non-controller options as needed), so we can refer to "attached elements" in the text and '#attached' in the code block?

2. +++ b/core/includes/common.inc
   @@ -613,17 +613,30 @@ function drupal_merge_attached(array $a, array $b) {
   + *   applicatons, there are three options:
   

   applicatons

3. +++ b/core/includes/common.inc
   @@ -613,17 +613,30 @@ function drupal_merge_attached(array $a, array $b) {
   + *   - Set up an event subscriber for KernelEvents::RESPONSE event. See
   

   for the

4. +++ b/core/includes/common.inc
   @@ -613,17 +613,30 @@ function drupal_merge_attached(array $a, array $b) {
   -
   

   unrelated change

Seems people want this gone and #26/ #28 points out why it makes little sense to keep this.

This still needs a change record draft. Also should this be postponed on #2477223: Refactor _drupal_add_html_head, drupal_get_html_head, _drupal_add_html_head_link into the attachments processor, remove from common.inc.?

1. +++ b/core/includes/common.inc
   @@ -613,10 +613,35 @@ function drupal_merge_attached(array $a, array $b) {
   + *   the response towards the end of the render process.  For example:
   

   Double space

2. +++ b/core/includes/common.inc
   @@ -613,10 +613,35 @@ function drupal_merge_attached(array $a, array $b) {
   + * @deprecated Will be removed before Drupal 8.0.0. Specify ['#attached']
   

s/['#attached']/attached/

So let's see about getting an answer

Bot 3128 was broken, it's now taken out of rotation.

There are three ways forward I can see as discussed with catch in IRC:

1. Keep BC and rename current drupal_process_attached to _drupal_process_attached, have the new drupal_process_attached do:

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

it won't be in global state, but somewhere on the RenderContext at least, so close enough.

2. Deprecate and throw an Exception() to show that this is really not supported anymore (compared to an empty shell), even google_analytics that uses it in a preprocess function could just use $variables['#attached'] instead. I also agree it was meant as internal and not public API, which is shown by the fact that the examples for this function documentation, never mention the function itself. Also leave _drupal_process_attached for internal core usage until its refactored in the other issue.

3. Rename the function to _drupal_process_attached and remove it before 8.0.0 - stating that this simply was an oversight, have a CR to explain the various ways the same can be achieved. As RenderContext's have been added as a suitable replacement that is compatible with Render and SmartCache caching, this should be fine.

All three have in common to rename current drupal_process_attached to _drupal_process_attached, so apparently I feel strongly about that ;).

Bumping to major

… I don't think it's true that D8 contrib code is using it. If there is such code, I'd bet that it's because they were doing it in D7 and haven't upgraded it since.

But that's the thing; contrib code actually is using it.
http://cgit.drupalcode.org/google_analytics/tree/google_analytics.module...

I think #41 needs consideration. Also, if we are considering this, it must have a change record.

If at all possible, an option that provides BC and deprecates for 9.0.0 is preferable (IMO).

#43: Ok. So then going with #41.1 would make sense.

However, note that if we'd remove it, all they'd need to do (and they could already do what is below TODAY):

$page['#attached']['html_head'][] = [
  …
];
drupal_process_attached($page);

to:

$variables['#attached']['html_head'][] = [
  …
];

That's it.

The reason you're able to find such use cases today is because Google Analytics was ported early, and after the initial port was working, they've (AFAIK) been keeping up in the minimal way possible: when it breaks, fix it. Less than 24 hours ago, the SafeMarkup fixes broke it: http://cgit.drupalcode.org/google_analytics/commit/?id=1e30fff.

So we could keep it, but we'd basically be actively supporting an API that follows D7 thinking, with lots of globals. Also note that it does not work for all types of attachments in HEAD! You cannot attach asset libraries or JS settings with it!. So we'd actively be supporting a broken API… because it was actually deprecated.

See #26.

I think this is something @xjm and @catch need to decide. I very much prefer to remove it because it was

1. always internal
2. intended to be deprecated which is why 'library' and 'drupalSettings' aren't supported anymore, precisely because we were working towards removing it
3. the alternatives are very easy
4. the alternatives are what you should've been doing even in D7 already!

But I also understand the desire to not break BC unless absolutely necessary. So if we keep BC, #41.1 is a good way. But we'll have to literally unset 'library' and 'drupalSettings' in that BC-compatible solution too, because otherwise we *also* change the behavior…

#47: To be precise, the idea of the underscore is just temporary until we can remove it. It obviously would also have a @deprecated, removed before 8.0.0 info in there, but would ease the transition of #2477223: Refactor _drupal_add_html_head, drupal_get_html_head, _drupal_add_html_head_link into the attachments processor, remove from common.inc..

Re #43 and others: If contrib doesn't need it, then we should be able write patches so that they don't use it. Opened #2562721: Remove call to drupal_process_attached() for Google Analytics, but I'm off out now if anyone else fancies tackling it.

Looking at the core usage

the nice bug https://api.drupal.org/api/drupal/core!themes!bartik!color!color.inc/8
https://api.drupal.org/api/drupal/core!themes!bartik!color!color.inc/8

// Put the logo path into JavaScript for the live preview.
$js_attached['#attached']['drupalSettings']['color']['logo'] = theme_get_setting('logo.url', 'bartik');
drupal_process_attached($js_attached);

but settings is not supported...

I was able to tackle #2562721: Remove call to drupal_process_attached() after all and the patch is trivial. If they are all that easy I don't mind submitting patches for all contrib projects!

@ianthomas_uk++ for #2562721: Remove call to drupal_process_attached()!

@Mile23++ for #2548991: Remove Bartik's erroneous use of drupal_process_attached(), add tests. — note that that is only possible because we have no test coverage for Color module.

That is the reason why GA has this code and because $varialbes has not worked a few weeks ago. It has nothing to do with keeping up in the minimal way possible.

That is very strange. I would remove better now than later e.g. in #2562721: Remove call to drupal_process_attached() i learned that attachments support #weight, but not if drupal_process_attached() is used. That is api inconsistency with render arrays and I guess we do not like to support this.

If this is kept for final we should at least document this important details and point to attachments with $variables.

I agree with #57, keeping it is not an option. I even discussed with catch on potentially making this critical as smart cache potentially increases the problematic-ness of the kept static values.

And especially because contrib code can be updated via:

s/drupal_process_attached(/\Drupal::service('renderer')->render(/g

I don't think this is a very disruptive change.

Overall I personally am for the following resolution:

- Rename current drupal_process_attached to _drupal_process_attached and mark it for removal / do-not-use / @internal.
- Add drupal_process_attached with the following content:

/**
  * @deprecated will be removed before 8.0.0.
  */
function drupal_process_attached() {
  throw new Exception('This function is no longer supported and will be removed before 8.0.0. Use \Drupal::service(\'renderer\')->render() instead. See [change record link] for more information.');
}

- Then remove drupal_process_attached() before cutting the final release tag.

Of course we could also leave that dead code in till 8.1.0 or whatever.

This gives helpful hints to someone still using that, while not holding up the conversion further, the _drupal_process_attached() will then just vanish once the conversion lands.

@xjm: Thoughts?

Why throw exception? maybe better assert?

An assert() does make more sense here...

@Mile23: Added example to https://www.drupal.org/node/2477223#comment-10299985

Tagging revisit-before-release as with smart_cache on this horribly fails for controllers:

#64:

It must literally be:

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

not renderRoot().

so 8.0 or 8.2? who's sign needed?
I see no reason to keep it atm

+++ b/core/includes/common.inc
@@ -574,10 +574,36 @@ function drupal_js_defaults($data = NULL) {
+ * @deprecated Will be removed before Drupal 8.0.0. Specify attached elements in

last patch

Given:

• the fact that the Google Analytics module is no longer using it, and in fact fixed a bug in the process (#2562721: Remove call to drupal_process_attached())
• #2564547: Remove calls to drupal_process_attached being RTBC (and being very trivial clean-up)
• #2477223: Refactor _drupal_add_html_head, drupal_get_html_head, _drupal_add_html_head_link into the attachments processor, remove from common.inc. being RTBC
• the argumentation in #67 and earlier comments
• choosing to keep drupal_process_attached() would mean we'd have to mimic in the BC layer the fact that 'library' and 'drupalSettings' are not supported by it because drupal_process_attached() in HEAD has not supported for more than seven months (and nobody has complained about it either, further indicating very few usages in contrib)
• the drupal_add_*() deprecated in favour of #attached CR (https://www.drupal.org/node/2160069) having sent a very clear message since DECEMBER 2013 that you should use #attached.

… I'm marking this RTBC.

It's now up to a framework manager to decide.

 Output: [PHP Fatal error:  Uncaught exception 'PDOException' with message 'SQLSTATE[HY000]: General error: 13 database or disk is full' in /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Database/StatementPrefetch.php:168

Testbot--

This too; reviewing now.

Thanks for the work getting GA updated and on the excellent patch documentation! #57 through #70 etc. are a compelling case to make this BC break now. So I am okay with this following the work that's now been done.

Can we get the change record in https://www.drupal.org/node/2565285 updated to include some of the same information in the deprecation message (or I guess just a link to https://www.drupal.org/node/2549159), and to clarify that it's going to be removed before 8.0.0? NW for that. Should be easy to fix.

Also, it would be better for accessibility and readability to link the change records to each other with meaningful link text, e.g., drupal_process_attached() is deprecated and will be removed before 8.0.0 (but that's non-blocking I guess.

interdiff looks good. RTBC-ing that and tagging so I don't forget.

Committed/pushed to 8.0.x, thanks!

@Mile23 yay!
published CR