Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
fromcommon.inc
Remaining tasks
- Follow up on
drupal_http_header_attributes()
: #2566619: Deprecate drupal_http_header_attributes() - Deprecate
drupal_process_attached()
: #2552865: Deprecate drupal_process_attached() for 8.0.0 and either throw an Exception or make it work via the RenderContext
User interface changes
API changes
Data model changes
Beta phase evaluation
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. |
Comment | File | Size | Author |
---|---|---|---|
#116 | 2477223_116.patch | 26.14 KB | ianthomas_uk |
| |||
#100 | interdiff.txt | 720 bytes | Mile23 |
#100 | 2477223_100.patch | 40.46 KB | Mile23 |
#95 | 2477223_95.patch | 40.31 KB | Mile23 |
#92 | interdiff.txt | 623 bytes | Mile23 |
Comments
Comment #1
a_thakur CreditAttribution: a_thakur at Srijan | A Material+ Company commentedComment #2
pjbaertI removed _drupal_add_html_head, drupal_get_html_head and _drupal_add_html_head_link & usages from common.inc
Comment #4
a_thakur CreditAttribution: a_thakur at Srijan | A Material+ Company commentedComment #5
j2r CreditAttribution: j2r commentedComment #6
j2r CreditAttribution: j2r commentedRemoved _drupal_add_html_head, drupal_get_html_head and _drupal_add_html_head_link & usages of these functions.
- wrong patch. Please ignore.
Comment #7
j2r CreditAttribution: j2r commentedComment #9
j2r CreditAttribution: j2r commentedComment #10
jaimeguzman CreditAttribution: jaimeguzman at Skilld commentedI 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 :
Comment #12
JeroenTHmm,
_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.
Comment #13
Mile23Working 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. :-)
Comment #14
andypostSo maybe better to split this issues on per function basis?
Comment #15
Wim Leers#2467759: Refactor drupal_process_attached() so it doesn't depend on drupal_add_http_header() just landed.
Comment #16
Mile23This 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.Comment #17
Wim LeersThanks 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.s/head//
s/http/HTTP/
s/settings/options/
s/sections/keys of #attached/
Agreed that this needs extra (well… *any* probably…) test coverage
This does more merging, so especially needs test coverage.
Comment #19
andyposttests show that we have solid integration coverage
+1 on approach, suppose we need @deprecated issue on drupal_http_header_attributes asap
Comment #20
Mile23If 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 callsdrupal_process_attached()
. :-)Comment #21
Mile23Still 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. :-)
Comment #23
Mile23Getting 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()
.Comment #25
Wim LeersWOOOHOOOOOOOOOO!
Great work here :)
Comment #26
Mile23Ok, so the only failure is BookTest, because I don't know enough about preprocessing for templates.
Comment #27
andypostGreen!!!
book issue is separate bug #1338858: Include theme print stylesheets in Book printer friendly export
Comment #28
Mile23A tiny bit of cleanup, some doc changes.
Comment #29
Mile23A 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.
Comment #31
Mile23Thou Shalt Not Change The Order Of Feed Link Attributes. (Because the test uses regex.)
Comment #32
dawehnerIs there no way to remove this bit of code? Can't we also unset the values?
Nitpick: you could remove the break statements ...
We would use #2550945: Add Html::escape() in the future. What about adding a todo?
Can we remove that now? Just curious
Did you considered to use \Drupal::theme() instead?
Comment #33
Wim LeersGreat docs updates here. :)
I think the "it replace […]" phrase can be removed here; we never write Drupal 7 comparisons in our documentation.
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 :)
Nit: Trailing spaces.
s/
list (
/list(
/s/attached/attachments/
Comment #34
Wim LeersWow, 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.
Comment #35
Mile23I'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. :-)Comment #37
Mile23This 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().
Comment #39
Mile23Fixed 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.
Comment #40
andypostI'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
should be "// @todo clean-up in URL" but as this not used anymore could be removed
that's actually makes no sense to keep,
it does nothing about from its doc-block
Comment #41
Mile23Created an issue about deprecating/removing
drupal_process_attached()
: #2552865: Deprecate drupal_process_attached() for 8.0.0 and either throw an Exception or make it work via the RenderContextComment #42
Mile23Reroll after #2551989: Move replacing of placeholders from HtmlRenderer to HtmlResponseAttachmentsProcessor
Comment #44
Mile23Thanks for renaming everything, guys. :-)
Comment #45
xjmThe 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!
Comment #46
Mile23The 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 RenderContextOnce 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.
Comment #47
Mile23Though 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 theHtmlResponse
.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 tobook.module
to point to #1338858: Include theme print stylesheets in Book printer friendly export This issue might be postponed on that one.Comment #48
Mile23There's also the issue of
hook_html_head_alter()
#2555069: Remove invocation of hook_html_head_alter()Comment #49
Crell CreditAttribution: Crell at Palantir.net commentedxjm: 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.
Comment #50
Mile23In IRC @mixologic (sorry if I got that name wrong) mentioned that google analytics module uses
drupal_process_attached()
, which it does ingoogle_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
frompager.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 wayHtmlResponseAttachmentsTest
works. Since that's the case let's just leavedrupal_process_attached()
and not make them do it.Much as I want to.
Comment #51
catchPreprocess can use $variables['#attached'].
Comment #52
Mile23OK, 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? :-)
Comment #53
catchPretty 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.
Comment #54
Wim LeersI 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, becausedrupal_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), andHtmlResponseAttachmentsProcessor
, which was specifically designed to not depend on the staticness ofdrupal_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.
Comment #55
Mile23It would be super-awesome if someone could add a test to demonstrate the cache fail.
'Needs tests' tag is still there. :-)
Comment #56
Mile23So 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.
Comment #57
Mile23... Sure would be nice if someone reviewed this.
Comment #58
joelpittetHere's a review:
Can this get a @deprecated instead of @todo?
This is landed so can be resolved.
Any idea why it's wrong or how to resolve this @todo?
How does this test need to be improved?
Comment #59
Mile23Thanks.
#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.
Comment #60
Mile23Needed a reroll.
Comment #61
Fabianx CreditAttribution: Fabianx for Acquia commentedThis 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.:
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.
Comment #62
Mile23Please provide a test which illustrates this. See #54, #56.
That way we can make a solid argument for deprecation before 8.0.0.
Thanks.
Comment #63
Fabianx CreditAttribution: Fabianx for Acquia commentedI 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.
Comment #64
Wim LeersExactly what #63 says.
Many thanks for keeping this going, @Mile23, I promise faster reviews from now on.
Comment #65
Mile23OK, 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.
Comment #66
joelpittet@Mile23 question:
This shouldn't need a \ in the front of the function name does it?
Comment #67
Mile23OK, 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.
Comment #69
hass CreditAttribution: hass commented@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:
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:
Comment #70
Mile23@hass: Thanks.
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()Comment #71
hass CreditAttribution: hass commentedI 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 ifdrupal_process_attached()
is used with render arrays.Comment #72
Mile23So 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:
render_attached_test
module fromcore/modules/system/tests/modules
to justmodules
.DrupalProcessAttachedBlock
to be in the content region.And yet, the test will probably pass on the testbot.
Someone smarter than I will have to figure it out.
Comment #74
Mile23Bugfixes 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:
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.Comment #75
joelpittetLeft your tools on the floor.
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.
Comment #76
Mile23Well, no. :-)
Also, in an ideal world, none of this code will be used because
drupal_process_attached()
will be deprecated.Comment #79
Fabianx CreditAttribution: Fabianx for Acquia commentedThis 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 :).
Comment #80
Mile23@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.
Comment #82
Mile23OK, it doesn't throw that exception locally, but does on the testbot.
Comment #83
Wim LeersGreat 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()
: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 :)Comment #84
Mile23Thanks.
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
Comment #86
Mile23Updating 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
Comment #87
andypostIt looks good to go except "nit" and a way for contrib to provide own attachment types
needs \ to full name
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
that mostly about ability to process this kind of attachments
Comment #88
Mile23Thanks for the review.
That's a feature request outside the scope of this issue, which deals with deprecating some functions.
Nice catch.
Comment #89
Wim LeersThat 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++
Can be removed.
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.
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.
Comment #91
Wim LeersDrupalCI: 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.
Comment #92
Mile23+1 on all those changes, except:
We really need to explain that in the docblock.
Comment #93
Wim LeersWorks for me!
Comment #94
Fabianx CreditAttribution: Fabianx for Acquia commentedRTBC +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.
Comment #95
Mile23Reroll after #2564547: Remove calls to drupal_process_attached
Comment #96
Wim LeersStill looks good.
Comment #99
catchFail looks real:
Comment #100
Mile23I can't repro those fails.
Here's the more explicit test from before.
Comment #101
Wim Leers#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.
Comment #103
Mile23Again, I can't repro the failing test, which is
\Drupal\field\Tests\Migrate\d7\MigrateViewModesTest
.Possibly unrelated.
Comment #106
Mile23Speaking of which.... We'll need a re-roll after #2568511: Fix broken test: KernelTestBase::render
Comment #108
Wim LeersGreen again. Testbot--
Comment #109
hass CreditAttribution: hass commented@wim: does #101 mean #2474865: drupal_render() and service no longer attach is expected?
Comment #110
Wim Leers#109: I replied there.
Comment #111
catchTagging 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.
Comment #112
Mile23It's ready to go now, has backwards-compatibility, deprecates code to be removed before 8.0.0.
What's the hanging question?
Comment #113
catchFor a start
I'm not sure if we should add the backwards compatibility at allreading 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.Comment #114
ianthomas_ukAs 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.
Comment #116
ianthomas_ukSimple reroll because of #2552865: Deprecate drupal_process_attached() for 8.0.0 and either throw an Exception or make it work via the RenderContext, just the one hunk fixed
Comment #117
Mile23Boo, 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.Comment #118
catchCommitted/pushed to 8.0.x, thanks!
Comment #121
Mile23Is it really fixed? Or is that the testbot burping?
Comment #122
ianthomas_ukIt'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.
Comment #123
Mile23Wppt!
Now to remove it: #2554771: Remove deprecated drupal_process_attached()
Comment #124
Mile23The tests got left out in the reroll. :-)
#2571427: HtmlResponseAttachmentsProcessor tests misplaced during reroll