Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.Problem/Motivation
There is a @todo in the documentation of file_save_upload() which says:
@todo: move this logic to a service in https://www.drupal.org/node/2244513.
However, this function calls _file_save_upload_single(), which uses the file_system service. Moreover, this function seems perfectly fine according to #3101976: Add tests for file_save_upload trimming trailing . characters from filenames and move the test into SaveUploadTest.
Proposed resolution
This note is confusing since it suggests there is a better way than using this function, and thus it should be removed.
Remaining tasks
- Acknowledge whether this note is no longer relevant.
- If so, remove it (see attached patch).
User interface changes
None.
API changes
None.
Data model changes
None.
| Comment | File | Size | Author |
|---|---|---|---|
| #16 | drupal-file_save_upload_service-3178225-16.patch | 663 bytes | liam morland |
| #12 | drupal-file_save_upload_service-3178225-12.patch | 692 bytes | liam morland |
| #10 | drupal-file_save_upload_service-3178225-10.patch | 692 bytes | liam morland |
| #2 | todo_note_for_file_save_upload-3178225-2.patch | 663 bytes | fmb |
Comments
Comment #2
fmb commentedComment #3
fmb commentedComment #4
joachim commentedIt makes sense to me that this code should be moved to a service.
But #2244513: Move the unmanaged file APIs to the file_system service (file.inc) is about file.inc, which this function isn't in -- it says "Currently there are a number of low-level file operations in file.inc ..."
So the TODO should probably stay, but either:
a. find the right existing issue it should mention
b. create an issue if there isn't one already
and then update the TODO to reference that instead.
Comment #5
kim.pepperI created a new issue #3232248: Move _file_save_upload_single to a service and deprecate split off from #2940383: [META] Unify file upload logic of REST and JSON:API
Comment #9
liam morlandComment #10
liam morlandThis updates the @todo to point to this issue. Once this is committed, this issue can be changed to be about creating the service.
Comment #11
smustgrave commentedNot sure I see the benefit of updating a comment that this issue will be worked on, and working on it later.
But if we are just trying to update to a more relevant issue then a follow up should be opened to actually fix it and the comment pointed there.
Comment #12
liam morlandI have created #3370626: Convert file_save_upload() to a service. This patch points to that issue.
Comment #13
smustgrave commentedLGTM
Thanks!
Comment #14
liam morlandComment #15
catchI think we should just remove the @todo altogether.
Comment #16
liam morlandI'm OK with that, though having the @todo could save some time. A person sees this function, thinks it should be a service, and opens a ticket or do they see the @todo and visit the existing ticket? The cost of having the @todo is zero and there is a benefit.
Comment #17
smustgrave commentedDon't think I need the tests to finish to know this won't break anything.
Comment #20
catchI think the fact this issue is open with six people on it shows there was a cost to having the @todo. I think they're useful when an implementation has drawbacks but needs a stack of issues to improve, but not for saying procedural code should be OOP.
Committed/pushed to 11.x and cherry-picked to 10.1.x, thanks!
Comment #21
liam morlandAdjusting title to match the work that was actually done.