Allow additional keys in #attached

MR moves the supported types to an argument when constructing HtmlResponseAttachmentsProcessor, and checks the keys with an assertion. A module could modify the values with a Service Provider.

Using a service parameter to set the types would probably be a little easier for modules to modify than altering the service's argument.

Doing some code archaeology, the exception for undefined types was added by #2350877: Deprecate/rename drupal_add_feed(), drupal_add_html_head(), drupal_add_html_head_link(), drupal_add_http_header(), and allow to be set declaratively in #attached → , and it looks like part of the original reason for an exception was that drupal_process_attached() changed from using a callback function based on the key name, to a limited set of values mapped to corresponding internal functions. Some discussion of additional keys starts at comment #30, but the only know use case was the contrib libraries API which could transition to extending the core library API.

It was moved to the attachments processor as part of refactoring in #2350877: Deprecate/rename drupal_add_feed(), drupal_add_html_head(), drupal_add_html_head_link(), drupal_add_http_header(), and allow to be set declaratively in #attached → that removed drupal_process_attached, and the exception was kept because of an existing test for it (comment #39), but not otherwise justified.

I'd better keep to throw exception for security reasons, assertions are usually disabled

I don't think there should be a security concern here. It's already possible to put any extra keys/data in #attached, as long as you decorate or replace so that HtmlResponseAttachmentsProcessor::processAttachments() doesn't see them, like Big Pipe does. HtmlResponseAttachmentsProcessor itself only interacts with the keys it expects and any others would be ignored. Anything put in #attached is also done as part of creating a renderable element - I don't think it should ever see user input, and any module would be responsible for safely handling it's own data anyways after permitting a new key.

Attach Inline can't safely decorate the service because it still needs to modify a call to AttachedAssets::createFromRenderArray() to use its own extended AttachedAssets class which preserves the additional #attached items to be passed on to the CSS / JSS collection renderers (which it is able to decorate).

I've implemented an example of CSP using extra data in #attached here: https://git.drupalcode.org/project/csp/-/merge_requests/38/diffs?commit_.... It could do similar to Big Pipe to remove and restore it's additional key to hide it from HtmlResponseAttachmentsProcessor, but disabling the CSP module while a render element still includes the additional key would result in the exception being thrown.

I think the assertion is even excessive - it would only be a developer hint that they've spelled a key wrong or are missing a module to process the the item, but nothing would break as any unrecognized keys are ignored - I'm inclined to just remove the key check entirely.

There's TODO in AttachmentsInterface which could be resolved with the issue

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Rebased.

CSP is now working around core by decorating the core attachments processor service, but it would be nice to not have to do this, since it also requires any module implementing CSP's new options to do a moduleExists() check.

Can the issue summary be updated with current solution.

May also need a title update as it seems it's all deletions.

Removed "Define accepted keys" option from summary, update how CSP now uses #attached.

Slightly different title; "Don't prevent modules from using #attached for additional data" would mean the same thing.

Think this warrants a CR?

Drafted one just in case.

I think this should have a test to prove it works as expected, for a range of values.

Hi, I’ve added a test to ensure that custom keys in the #attached array, such as library and drupalSettings, are handled properly without causing any exceptions.

Kindly review the changes and please let me know if any further adjustments are required.

Thanks!

The test includes a couple keys previously supported by core, but not an unknown key (e.g. the csp example in the issue summary)

MR updated! Added new key for content security policy in tests. Needs review.

Test coverage appears to be added. Don't believe any outstanding questions.

I found an instance where the CSP module isn't able to prevent core from throwing an exception on the presence of an unexpected key in #attached by decorating the core service: 💬 Questions about adding nonces Active

If a render element uses a lazy builder, and the lazy builder adds the additional #attached property on its return value, the core service is the one that calls the the lazy builder after which it will throw an exception on the unsupported key. CSP doesn't have an opportunity to filter the key before core checks it.

The original idea to throw the exception here wasn't to prevent people from supporting different things, but to validate the contents of the render array. The current approach opens it up but means no validation at all.

I am wondering if we need something like a container parameter for allowed attached types, that the CSP module could then add to the list. That would retain the validation then.

Sounds like a good compromise @catch.

Another motivation was to prevent people from adding MB of js that we have to bubble around during rendering, so making it hard to add things was part of the goal of I remember correctly. Now it's less of an issue with lazy builders and all the other things we have available