Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Updated: Comment #N
Problem/Motivation
While reviewing #1815504: file_save_upload() should not alter the file URI string before validation, I found that file_save_upload() uses $file->filename and $file->uri in a watchdog message, which lead to erroneous watchdog messages:
72 12/Dec 14:09 file notice Upload error. Could not move uploaded file Object to destination Object.
(this is generated when drupal_move_uploaded_file() fails - which I manually forced to fail for this exercise).
Proposed resolution
Use the appropriate methods $file->getFilename() and $file->getFileUri(), respectively.
Remaining tasks
write patch and look for other occurences of $file->filename and $file->uri throughout core.
Beta phase evaluation
| Issue category | Bug |
|---|---|
| Unfrozen changes | Unfrozen because it only changes how file properties are accessed, and fixes bugs in watchdog messages. |
| Prioritized changes | The main goal of this issue is fixing a bug in watchdog notices. |
| Comment | File | Size | Author |
|---|---|---|---|
| #12 | drupal-file_upload_test_addition-2155245-12.patch | 4.42 KB | ChristianAdamski |
| #10 | drupal-file_upload_test_addition-2155245-10.patch | 3.3 KB | ChristianAdamski |
| #5 | 2155245-5.patch | 1.12 KB | areke |
| #2 | 2155245_2_file_filename_uri.patch | 1.11 KB | scor |
Comments
Comment #1
scor CreditAttribution: scor commentedComment #2
scor CreditAttribution: scor commentedI also fixed another instance that Peter had found in #1815504: file_save_upload() should not alter the file URI string before validation.
With this patch, the watchdog message reads:
Comment #3
scor CreditAttribution: scor commentedGrepping through core, with
I find lots of results. Surely there must be some legitimate use cases for using the property directly, but can someone familiar with D8 file entities explain where to draw the line?
Comment #4
jhedstromI'd say all of them should use the method if available. I'm surprised those properties aren't protected.
Comment #5
areke CreditAttribution: areke commentedRerolled patch.
Comment #6
areke CreditAttribution: areke commentedComment #7
jhedstromThis looks good. I added the beta phase evaluation to the summary.
I think we can tackle remaining occurrences of the direct property usage later (since this change fixes a bug, while the others aren't urgent).
Comment #8
penyaskitoRemoving Needs reroll tag for now. Probably this should have tests, but I will wait for others to chime in.
Comment #9
alexpottIf we are fixing a bug we should have a test.
Comment #10
ChristianAdamski CreditAttribution: ChristianAdamski commentedThis patch does two things:
1.) It adds a new test that deliberately fails moving a file upload by targeting a non-writable directory, triggering and failing move_uploaded_file() and therefor causing the aforementioned log message to be written. It then checks for that log message being present.
NOTICE: without the patches above, this will fail. That's the purpose :)
2.) This patch additionally introduces a check in the tested form, if the const SIMPLETEST_COLLECT_ERRORS should be set, to suppress the unavoidable PHP WARNINGs by move_uploaded_file() failing.
Comment #11
BerdirThe warnings trick is a bit nasty, but I have no better idea right now.
Can you upload a combined patch with the test and fix as well, so we can see that the test is now green?
Comment #12
ChristianAdamski CreditAttribution: ChristianAdamski commentedThere you go.
Comment #14
tstoecklerYes, I don't much like it either, but by wrapping it in the state check it's at least fairly self contained. And I looked around for quite a while and there really isn't any cleaner way of doing this.
There's a lot of room for the error handling in simpletest but in the scope of this issue I really cannot find anything to complain about this patch.
Comment #15
tstoecklerOh and thanks a lot for figuring this out @ChristianAdamski, quite a though one! :-)
Comment #16
alexpottCommitted 0c1276b and pushed to 8.0.x. Thanks!
Thanks for adding beta evaluation to the issue summary.