Problem/Motivation

TextItem contains a processed property
This property is a computed field built with the TextProcessed class
Calling $entity->text_field->processed will return you the filtered text, i.e. with all filters applied.

At this point if you're using $entity->text_field->processed in a render array, you should also take care to collect its cacheable-metadata. Because TextProcessed implements CacheableDependencyInterface, you can do that like so

$cache = new BubbleableMetadata();
$a_render_array = [
  '#theme' => 'biscuits',
  '#cookies' => $entity->text_field->processed,
];
$cache->addCacheableDependency($entity->text_field->processed);
$cache->applyTo($a_render_array);

However a the processed protected property on TextProcessed is an instance of FilterProcessResult, which extends from BubbleableMetadata, not just CacheableMetadata

This means there are #attachments inside it too, not just #cache entries.

There is no way to access those attachments and add them to your render array.

This is minor because you can work around this by doing this instead

$a_render_array = [
  '#theme' => 'biscuits',
  '#cookies' => [
    '#type' => 'processed_text',
    '#text' => $entity->text_field->value,
    '#format' => $entity->text_field->format,
];

Steps to reproduce

Write a filter plugin that adds Javascript or CSS (or any #attached)
Add an input format with that filter plugin enabled
Create an entity with a text field on it
Access the contents of the field for use in a render array using $entity->text_field->processed
Note that your render array won't include the attachments

Proposed resolution

Have TextProcessed also implement AttachmentsInterface

Remaining tasks

Implement the interface
Add a test

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3280235

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

larowlan created an issue. See original summary.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

smustgrave made their first commit to this issue’s fork.

smustgrave opened merge request !12689

smustgrave’s picture

smustgrave commented
Status: Active » Needs work

Added the suggestion but still NW for the test coverage which I'm not entirely sure would look like

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.

smustgrave’s picture

smustgrave commented
Status: Needs work » Needs review

So wasn't sure how to write a test for this so I did ask Claude code (acknowledging use of AI). I did cleanup the output some as it was adding test coverage for AttachmentsInterface specifically and this is more about making sure TextProcessed works with AttachmentsInterface.

dcam’s picture

dcam commented
Status: Needs review » Needs work

Overall it looks OK to me. If the issue is that we can't get attachments from TextProcessed objects, then the test shows that we can with this change.

I left one suggestion on the MR to fix an implicit any() expectation on the mocked object.

smustgrave’s picture

smustgrave commented
Status: Needs work » Needs review

Applied the suggestion.

dcam’s picture

dcam commented
Status: Needs review » Reviewed & tested by the community

Looks good to me.

godotislate’s picture

godotislate
he/him
commented
Status: Reviewed & tested by the community » Needs work

Couple things:
First, this example in the IS:

$cache = new CacheableMetadata();
$a_render_array = [
  '#theme' => 'biscuits',
  '#cookies' => $entity->text_field->processed,
];
$cache->addCacheableDependency($entity->text_field->processed);
$cache->applyTo($a_render_array);

Since $cache is an instance of CacheableMetadata, and not BubbleableMetdata, it can't capture attachments anyway. So I assume this should be:

$cache = new BubbleableMetadata();
$a_render_array = [
  '#theme' => 'biscuits',
  '#cookies' => $entity->text_field->processed,
];
$cache->addCacheableDependency($entity->text_field->processed);
$cache->applyTo($a_render_array);

Second, with the changes in the MR, the attachments in the TextProcessed processed property of a TextItem field item can be captured, but the IS mentions that the attachments in the FilterProcessResult processed property of the TextProcessed data type are not captured. (Yes, this is probably a little confusing.) The MR changes AFAICT do not capture these. In order to do so, I think getAttachments() needs to be overridde in TextProcessed so that it's something like:

  /**
   * {@inheritdoc}
   */
  public function getAttachments() {
    $this->addAttachments($this->processed->getAttachments());
    return $this->attachments;
  }

And test coverage for this as well.

a.dmitriiev made their first commit to this issue’s fork.

a.dmitriiev’s picture

a.dmitriiev commented
Priority: Minor » Normal
Status: Needs work » Needs review

Similar problem was reported here #3584177: highlight js doenst highlight code in Drupal cms 2.x D11, so I am raising the priority to "Normal" as it seems it affects all sites that use canvas. In the referenced issue the same fix was suggested. I have also added a kernel test with FilterProcessResult.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Feedback looks good to me.

godotislate closed merge request !12689

  • godotislate committed b5b29d41 on main 
    fix: #3280235 Calling code can not collect bubbled attachments from...

  • godotislate committed 3460ecfb on 11.x 
    fix: #3280235 Calling code can not collect bubbled attachments from...

  • godotislate committed f05f7abd on 11.4.x 
    fix: #3280235 Calling code can not collect bubbled attachments from...
godotislate’s picture

godotislate
he/him
commented
Version: main » 11.4.x-dev
Issue summary: View changes
Status: Reviewed & tested by the community » Fixed

Committed and pushed b5b29d4 to main, 3460ecf to 11.x, and f05f7ab to 11.4.x. Thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

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