Problem/Motivation
drupal_process_attached()
will be deprecated, either for removal before 8.0.0 or 9.0.0. Either way, references to it should be removed from core as much as possible (or, in the case of 8.0.0, completely).
According to https://api.drupal.org/api/drupal/core!includes!common.inc/function/call... there are 7 calls to drupal_process_attached().
One is in a test specifically for drupal_process_attached()
and should be kept for as long as we have the function.
One is being handled in #2548991: Remove Bartik's erroneous use of drupal_process_attached(), add tests.
Removing the other five would help to prove that drupal_process_attached()
is unnecessary. These are:
- AddFeedTest::testBasicFeedAddNoTitle in core/modules/system/src/Tests/Common/AddFeedTest.php
- HtmlResponseAttachmentsProcessor::processAttachments in core/lib/Drupal/Core/Render
- KernelTestBase::render in core/tests/Drupal/KernelTests/KernelTestBase.php
- KernelTestBase::render in core/modules/simpletest/src/KernelTestBase.php
- ViewsFormBase::ajaxFormWrapper in core/modules/views_ui/src/Form/Ajax/ViewsFormBase.php
There are also numerous references to drupal_process_attached()
in documentation, which should be changed.
Proposed resolution
Remove usages of drupal_process_attached()
.
Remaining tasks
Remove drupal_process_attached()
itself. #2554771: Remove deprecated drupal_process_attached()
User interface changes
API changes
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#4 | interdiff.txt | 1.23 KB | Mile23 |
#4 | 2564547_4.patch | 3.78 KB | Mile23 |
#3 | 2564547_3.patch | 3.66 KB | Mile23 |
Comments
Comment #2
Mile23#2477223: Refactor _drupal_add_html_head, drupal_get_html_head, _drupal_add_html_head_link into the attachments processor, remove from common.inc. does some of this, but it's an easy re-roll either way. Let's concentrate on removing all usages here.
Comment #3
Mile23Everything's easy except for
ViewsFormBase
, which has no tests I can find. I mean, it's easy, but I can't verify that it's actually easy. :-) Maybe the testbot will tell us.AddFeedTest
merely replicates the changes toKernelTestBase
. Given this, it might be worth switchingAddFeedTest
to subclassKernelTestBase
, but that's out of scope here.This removes all usages other than the one in Bartik and the
HtmlResponseAttachmentsProcessor
which needs it for BC: #2477223: Refactor _drupal_add_html_head, drupal_get_html_head, _drupal_add_html_head_link into the attachments processor, remove from common.inc.Comment #4
Mile23Missed this. :-)
Comment #5
ianthomas_ukThe old test was rather lacking as far as testing coverage of drupal_process_attached() goes, but we shouldn't actually remove it if we're not removing the drupal_process_attached() itself.
Your new test would sit better in AttachedAssetsTest, maybe called testInvalidAttachmentType?
The other code changes look sensible, although I haven't tested them yet.
You haven't touched HtmlResponseAttachmentsProcessor::processAttachments or the simpletest version of KernelTestBase.
There are lots of @see references to drupal_process_attached(), most of these can likely just be removed.
I've made a start on a change record (attached)
Comment #6
Mile23The patch only changes the description of the test to be more accurate. It doesn't test drupal_process_attached() at all; it tests a behavior related to illegal #attached types.
I didn't add a new test.
Also #2477223: Refactor _drupal_add_html_head, drupal_get_html_head, _drupal_add_html_head_link into the attachments processor, remove from common.inc. adds a ton of tests for the behavior of the attachment processor; maybe we should change tests there, and only remove references to
drupal_process_attached()
here.@see is still appropriate until we get to #2554771: Remove deprecated drupal_process_attached()
Nice change record; there's also this one: https://www.drupal.org/node/2549159
Comment #7
Mile23Comment #8
andypostPrevious code catch all exceptions, maybe better to catch more specific exception?
Comment #9
Mile23That is exactly the reason I made that change.
drupal_process_attached()
andHtmlResponseAttachmentsProcessor
both throwLogicException
in this circumstance. The change improves the test.Comment #10
Wim LeersComment #11
Wim LeersComment #12
alexpottCommitted 8b3b8c6 and pushed to 8.0.x. Thanks!
Comment #14
ianthomas_ukIt looks to me that Drupal\KernelTests\KernelTest->render() will always pass after this patch. I've uploaded a fix to #2568511: Fix broken test: KernelTestBase::render.