Problem/Motivation

Drupal\Core\Render\AttachmentsInterface is basically as bad as the legendary ContextInterface was "interface for context".
It doesn't document what is in the $attachments array neither does it elaborate on the difference between set() and add().

Steps to reproduce

Open AttachmentsInterface . Try to figure it out what it does. Try to figure out the difference between set and add.

Proposed resolution

1) Move AttachmentsResponseProcessorInterface::processAttachments() doxygen into the AttachmentsInterface class doxygen .
2) refer to the class doxygen in every method doxygen and processAttachments() too.
3) Explain set vs add.

Remaining tasks

Review

User interface changes

Not relevant.

API changes

Not relevant.

Data model changes

Not relevant.

Release notes snippet

Not relevant.

Issue fork drupal-3451136

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:

Comments

Ghost of Drupal Past created an issue. See original summary.

andypost’s picture

Title: AttachmentsInterface is not documented » Resolve @todo in AttachmentsInterface
Category: Bug report » Task
Issue summary: View changes
Issue tags: +Documentation, +Needs documentation

There's TODO in class description which missing related issue, so documentation of the methods looks related

ghost of drupal past’s picture

Title: Resolve @todo in AttachmentsInterface » AttachmentsInterface is not documented

I am sorry but that's not what this issue is about. That TODO has nothing to do with documentation, AttachedAssetsInterface doesn't document what is in the $attachments array neither does it elaborate on the difference between set and add. Also given #3448503: Allow modules to use #attached for additional data I am not sure whether that TODO will ever resolve.

andypost’s picture

Title: AttachmentsInterface is not documented » Improve documentation of AttachmentsInterface methods
Issue summary: View changes

Thank you, updated IS from #3 but the title should be more actionable/precise about what to do

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

gapple’s picture

Made an MR with a start to the changes, including some fixes to the documentation - the example of adding a library was wrong, and the list of keys used by core was incomplete.

andypost’s picture

Status: Active » Needs review
smustgrave’s picture

Curious what's next steps for this one?

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

quietone’s picture

Issue summary: View changes

Updated for coding standard fixes and then added @see. The changes for coding standards required changes to the list of keys so that section needs review as it is no longer simply a move of code from one file to another.

ghost of drupal past’s picture

Awesome. Don't we want to add a @see to the param doxygen on the interface of $attachments / return value of get? The class doxygen is not always immediately visible -- for example on api.drupal.org , in an IDE popup and more.

quietone’s picture

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

@Ghost of Drupal Past, Ah, that is step 2 in the remaining tasks. I didn't do that because it just seems odd to have an @see directing to the same file and for file that is only 75 lines. Sorry, I should have commented on that. But, I am happy to be convinced otherwise. Or, we get this improvement in and punt step 2/#12 to another issue. The later is my preference as I prefer incremental doc improvements over the other options.

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Current change reads fine

with regards to

punt step 2/#12 to another issue.

do we want a follow up?

  • longwave committed 723d4d71 on 10.3.x
    Issue #3451136 by quietone, gapple, ghost of drupal past: Improve...

  • longwave committed db4e4e54 on 10.4.x
    Issue #3451136 by quietone, gapple, ghost of drupal past: Improve...

  • longwave committed a21edc21 on 11.0.x
    Issue #3451136 by quietone, gapple, ghost of drupal past: Improve...

  • longwave committed d820d8ed on 11.x
    Issue #3451136 by quietone, gapple, ghost of drupal past: Improve...

longwave’s picture

Version: 11.x-dev » 10.3.x-dev
Component: base system » documentation
Status: Reviewed & tested by the community » Fixed
Issue tags: -Documentation, -Needs documentation

As someone who's opened this file in the past and been confused by this, incremental improvements are welcome. Feel free to open followups to improve this further if anyone thinks it is necessary.

Backported down to 10.3.x as a docs-only fix.

Committed and pushed d820d8ed3e to 11.x and a21edc2188 to 11.0.x and db4e4e5426 to 10.4.x and 723d4d718a to 10.3.x. Thanks!

Status: Fixed » Closed (fixed)

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

wim leers’s picture

I was one of the people who introduced these 2 files in #2407195: Move attachment processing to services and per-type response subclasses, apologies for not getting the docs better at the time 🫣 (Looks like a simple location swap would've prevented most pain.)