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
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:
- 3451136-attachments-interface-docs
changes, plain diff MR !8268
Comments
Comment #2
andypostThere's TODO in class description which missing related issue, so documentation of the methods looks related
Comment #3
ghost of drupal pastI 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.
Comment #4
andypostThank you, updated IS from #3 but the title should be more actionable/precise about what to do
Comment #7
gappleMade 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.
Comment #8
andypostComment #9
smustgrave commentedCurious what's next steps for this one?
Comment #11
quietone commentedUpdated 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.
Comment #12
ghost of drupal pastAwesome. 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.
Comment #13
quietone commented@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.
Comment #14
smustgrave commentedCurrent change reads fine
with regards to
do we want a follow up?
Comment #20
longwaveAs 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!
Comment #22
wim leersI 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.)