Remove calls to drupal_process_attached [#2564547]

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
#4	8.0.x: PHP 5.5 & MySQL 5.5 13,813 pass
#3	2564547_3.patch	3.66 KB	Mile23
#3	8.0.x: PHP 5.5 & MySQL 5.5 13,727 pass

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

7 September 2015 at 21:18

ianthomas_uk created an issue. See original summary.

Comment #2

Mile23

English

Seattle, WA

CreditAttribution: Mile23 as a volunteer commented 7 September 2015 at 21:59

Issue summary:	View changes
Related issues:		+#2554771: Remove deprecated drupal_process_attached()

#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

Mile23

English

Seattle, WA

CreditAttribution: Mile23 as a volunteer commented 7 September 2015 at 23:24

Status:

Active

» Needs review

File	Size
2564547_3.patch	3.66 KB
8.0.x: PHP 5.5 & MySQL 5.5 13,727 pass

Everything'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 to KernelTestBase. Given this, it might be worth switching AddFeedTest to subclass KernelTestBase, 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

Mile23

English

Seattle, WA

CreditAttribution: Mile23 as a volunteer commented 7 September 2015 at 23:25

File	Size
2564547_4.patch	3.78 KB
8.0.x: PHP 5.5 & MySQL 5.5 13,813 pass
interdiff.txt	1.23 KB

Missed this. :-)

Comment #5

ianthomas_uk

he/him

English

Brighton, UK

CreditAttribution: ianthomas_uk as a volunteer commented 8 September 2015 at 22:28

Status:

Needs review

» Needs work

+++ b/core/modules/system/src/Tests/Common/RenderTest.php
@@ -53,17 +53,17 @@ function testDrupalRenderThemePreprocessAttached() {
-   * Tests drupal_process_attached().
+   * Tests that we get an exception when we try to attach an illegal type.

The 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

Mile23

English

Seattle, WA

CreditAttribution: Mile23 as a volunteer commented 9 September 2015 at 03:09

+++ b/core/modules/system/src/Tests/Common/RenderTest.php
@@ -53,17 +53,17 @@ function testDrupalRenderThemePreprocessAttached() {
-   * Tests drupal_process_attached().
+   * Tests that we get an exception when we try to attach an illegal type.
The 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.

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

Your new test would sit better in AttachedAssetsTest, maybe called testInvalidAttachmentType?

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

Mile23

English

Seattle, WA

CreditAttribution: Mile23 as a volunteer commented 10 September 2015 at 03:03

Status:

Needs work

» Needs review

Comment #8

andypost

he/him

Russian

CreditAttribution: andypost as a volunteer and at Skilld commented 10 September 2015 at 21:48

+++ b/core/modules/system/src/Tests/Common/RenderTest.php
@@ -53,17 +53,17 @@ function testDrupalRenderThemePreprocessAttached() {
-    catch (\Exception $e) {
+    catch (\LogicException $e) {
       $this->pass("Invalid #attachment 'drupal_process_states' not allowed");

Previous code catch all exceptions, maybe better to catch more specific exception?

Comment #9

Mile23

English

Seattle, WA

CreditAttribution: Mile23 as a volunteer commented 10 September 2015 at 22:00

That is exactly the reason I made that change.

drupal_process_attached() and HtmlResponseAttachmentsProcessor both throw LogicException in this circumstance. The change improves the test.

Comment #10

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 11 September 2015 at 06:58

Status:

Needs review

» Reviewed & tested by the community

Comment #11

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 11 September 2015 at 07:01

Issue tags:

+Quick fix

Comment #12

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott at Chapter Three commented 13 September 2015 at 16:45

Status:

Reviewed & tested by the community

» Fixed

Committed 8b3b8c6 and pushed to 8.0.x. Thanks!

Comment #13

13 September 2015 at 16:45

alexpott committed 8b3b8c6 on 8.0.x

Issue #2564547 by Mile23, ianthomas_uk: Remove calls to...

Comment #14

ianthomas_uk

he/him

English

Brighton, UK

CreditAttribution: ianthomas_uk as a volunteer commented 14 September 2015 at 22:21

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