Problem/Motivation

#2477223: Refactor _drupal_add_html_head, drupal_get_html_head, _drupal_add_html_head_link into the attachments processor, remove from common.inc. left drupal_process_attached() as a no-op.

#2552865: Deprecate drupal_process_attached() for 8.0.0 and either throw an Exception or make it work via the RenderContext marked it as deprecated.

This issue will do the actual removal.

Proposed resolution

  • Remove the function drupal_process_attached().
  • Change all in-code documentation to tell the reader that they should just use render arrays to attach headers and head tags.

Remaining tasks

User interface changes

API changes

Data model changes

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because it is removing an obsolete API (Could argue for Bug because the API being removed is inconsistent, see #2552865-57: Deprecate drupal_process_attached() for 8.0.0 and either throw an Exception or make it work via the RenderContext onwards)
Prioritized changes The main goal of this issue is removing code already deprecated for 8.0.0
Disruption Not disruptive for core, because all uses have been removed.

Slightly disruptive for contributed and custom modules because it will require a small BC break.
CommentFileSizeAuthor
#15 interdiff_8.patch15.22 KBmile23
#15 so_long_drupal_process_attached_2554771_15.patch23.56 KBmile23
#8 so_long_drupal_process_attached-2554771-8.patch8.88 KBwim leers

Comments

Mile23 created an issue. See original summary.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Active » Postponed
Parent issue: » #2205673: [META] Remove all @deprecated functions marked "remove before 8.0"
Related issues: +#2552865: Deprecate drupal_process_attached() for 8.0.0 and either throw an Exception or make it work via the RenderContext, +#2477223: Refactor _drupal_add_html_head, drupal_get_html_head, _drupal_add_html_head_link into the attachments processor, remove from common.inc.

Immediately set postponed on the decision to deprecate: #2552865: Deprecate drupal_process_attached() for 8.0.0 and either throw an Exception or make it work via the RenderContext

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Version: 8.0.x-dev » 9.x-dev

#2552865: Deprecate drupal_process_attached() for 8.0.0 and either throw an Exception or make it work via the RenderContext says to remove in D9, changing version.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Related issues: +#2548991: Remove Bartik's erroneous use of drupal_process_attached(), add tests.

Related issue which removes a usage of drupal_proccess_attached(): #2548991: Remove Bartik's erroneous use of drupal_process_attached(), add tests.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Version: 9.x-dev » 8.0.x-dev
Issue tags: +@deprecated

OK, so #2552865: Deprecate drupal_process_attached() for 8.0.0 and either throw an Exception or make it work via the RenderContext is in, that means this is a go for removal before 8.0.0 release. :-)

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

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Postponed » Active
ianthomas_uk’s picture

ianthomas_uk
he/him
Primary language English
Location Brighton, UK
commented

When rolling a patch for this, don't forget to include the many @see references that are scattered around the codebase.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Active » Needs review
StatusFileSize
new so_long_drupal_process_attached-2554771-8.patch8.88 KB

I've spent many, many, many hours on the many, many steps it took to get rid of the globals-littered craziness that was drupal_process_attached(), so the fact that we've reached this point and I was able to roll the attached patch is extremely satisfactory :)

One less loose end!

One less bit of legacy!

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Needs review » Needs work

so_long_drupal_process_attached

:-)

+++ b/core/modules/views/src/ViewExecutable.php
@@ -399,7 +399,7 @@ class ViewExecutable implements \Serializable {
+   * @see \Drupal\Core\Render\AttachmentsResponseProcessorInterface::processAttachments()

We could take the time and improve the API documentation at processAttachments(). It only tells the top-level keys, and not how the data should be structured.

Also, it could say 'Don't call this directly!' :-)

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Also, it could say 'Don't call this directly!' :-)

The signature is processAttachments(AttachmentsInterface $response) — if people call it directly with their render arrays, it'll fail miserably :)

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented 
$response = new HtmlResponse();
$response->addAttachments($render['#attached']);
$process = new HtmlResponseAttachmentsProcessor();
$content = $process->processAttachments($response);
// Wait, what?
mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Needs work » Postponed

#2571427: HtmlResponseAttachmentsProcessor tests misplaced during reroll will add some references to drupal_process_attached(). We should postpone on it.

ianthomas_uk’s picture

ianthomas_uk
he/him
Primary language English
Location Brighton, UK
commented
Issue summary: View changes
Status: Postponed » Reviewed & tested by the community
Issue tags: +rc deadline

The references to drupal_process_attached in #2571427: HtmlResponseAttachmentsProcessor tests misplaced during reroll are specifically to test this function, but the function is obsolete and should be removed. This issue is rc deadline, but the tests are not, so I don't see why we need to risk missing the rc for those. Rerolling that issue after this has gone in should be straight forward. I've reviewed the patch and that issue is also RTBC now, so the maintainers can pick which they'd like to commit and which needs to be rerolled.

Added beta evaluation

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 8: so_long_drupal_process_attached-2554771-8.patch, failed testing.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Needs work » Needs review
StatusFileSize
new so_long_drupal_process_attached_2554771_15.patch23.56 KB
new interdiff_8.patch15.22 KB

Removes references to drupal_process_attached(), including the tests. Changes the names of some test classes to reflect this.

Oops... The interdiff ended up with the .patch extension. Fumblefingers!

ianthomas_uk’s picture

ianthomas_uk
he/him
Primary language English
Location Brighton, UK
commented
Status: Needs review » Reviewed & tested by the community

Does what #15 says (removing the references that were added by #2571427: HtmlResponseAttachmentsProcessor tests misplaced during reroll)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 15: interdiff_8.patch, failed testing.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Reviewed & tested by the community

The fail was just for #15's interdiff, not for the actual patch.

Back to RTBC.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

KABOOOOOOOOOOOOOOOOM!

Committed 1844088 and pushed to 8.0.x. Thanks!

  • alexpott committed 1844088 on 8.0.x 
    Issue #2554771 by Mile23, Wim Leers: Remove deprecated...
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

https://www.drupal.org/node/2565285 needs to be updated and re-published.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented 
     _.-^^---....,,--
 _--                  --_
<                        >)
|                         |
 \._                   _./
    ```--. . , ; .--'''
          | |   |
       .-=||  | |=-.
       `-=#$%&%$#=-'
          | ;  :|
 _____.,-#%&$@%#&#~,._____

WOOOHOOOO!

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

CR updated.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented

Just one more issue before we can feel good about it... :-)

#2568511: Fix broken test: KernelTestBase::render

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.