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.
Problem/Motivation
PHP's unlink()
is broken on Windows, as it can fail to remove a file when it has a read-only flag set. And as it seems the most files handled by MTimeProtectedFastFileStorage
have the read-only flag set we should use drupal_unlink()
instead plain unlink()
.
Proposed resolution
Use drupal_unlink()
instead plain unlink()
in MTimeProtectedFastFileStorage
.
Remaining tasks
Review
User interface changes
none.
API changes
none.
Comment | File | Size | Author |
---|---|---|---|
#19 | MTimeProtectedFastFileStorage-1849570-19.patch | 5.8 KB | marcingy |
#19 | interdiff.txt | 4.5 KB | marcingy |
#16 | unlink.jpg | 48.2 KB | smiletrl |
#15 | MTimeProtectedFastFileStorage-1849570-15.patch | 5.98 KB | das-peter |
#15 | interdiff-1849570-14-15.txt | 3.29 KB | das-peter |
Comments
Comment #1
Hydra CreditAttribution: Hydra commentedSince this is exactly the purpose of drupal_unlink, this should be adjusted as suggested from das-peter. Thanks for the patch!
Comment #2
chx CreditAttribution: chx commentedThanks for the patch. Alas the solution picked only works because we have used a stopgap drupal_bootstrap call there to allow for things like this which I am removing in #1849004: One Service Container To Rule Them All. Please do not add more calls into file.inc, instead remove them where you find them.
Repurposing issue.
Comment #3
das-peter CreditAttribution: das-peter commentedAdded a protected method
MTimeProtectedFastFileStorage::unlink()
with the same code asdrupal_unlink()
uses.According to my regex search
(drupal_basename|drupal_chmod|drupal_dirname|drupal_mkdir|drupal_move_uploaded_file|drupal_realpath|drupal_rmdir|drupal_tempnam|drupal_unlink|file_build_uri|file_create_filename|file_create_url|file_default_scheme|file_delete|file_delete_multiple|file_destination|file_directory_temp|file_download|file_ensure_htaccess|file_get_mimetype|file_get_stream_wrappers|file_munge_filename|file_prepare_directory|file_save_htaccess|file_save_upload|file_scan_directory|file_stream_wrapper_get_class|file_stream_wrapper_get_instance_by_scheme|file_stream_wrapper_get_instance_by_uri|file_stream_wrapper_uri_normalize|file_stream_wrapper_valid_scheme|file_transfer|file_unmanaged_copy|file_unmanaged_delete|file_unmanaged_delete_recursive|file_unmanaged_move|file_unmanaged_save_data|file_unmunge_filename|file_upload_max_size|file_uri_scheme|file_uri_target|file_valid_uri)
the call to
file_save_htaccess()
is the only dependency left to file.incHowever I'm not sure how to solve that, shall we duplicate more code? :|
Comment #4
das-peter CreditAttribution: das-peter commentedNext version after a talk in IRC with aspiliciious & chx.
I changed quite a bit now (besides the coding standard fixes):
FileStorage::unlink()
, this basically does the same asdrupal_unlink()
- ensure the file can be deleted.FileStorage::deleteAll()
now uses SPL iterators and objects to do it's job. Behaviour is the same as infile_unmanaged_delete_recursive()
. However, I'm not sure if the watchdog part is necessary.MTimeProtectedFastFileStorage::saveHtaccess()
is the replacement forfile_save_htaccess()
, I stripped all the stuff I thought is unrelated.Comment #5
das-peter CreditAttribution: das-peter commentedIRC Log:
Done as said.
Comment #7
das-peter CreditAttribution: das-peter commentedAdded check if dir is a dot.
Comment #9
chx CreditAttribution: chx commented#7: MTimeProtectedFastFileStorage-1849570-7.patch queued for re-testing.
Comment #11
chx CreditAttribution: chx commentedThat's an interesting fail.... Note that the proper syntax would be "use RecursiveIteratorIterator;" but also we decided not to do those for root classes just put the root wherever they are used. As for the htaccess code, see #1849004: One Service Container To Rule Them All for the minimal implementation.
Comment #12
das-peter CreditAttribution: das-peter commentedNext re-roll:
- Removed
use
for SPL classes.- Changed htaccess code to the example in #1849004: One Service Container To Rule Them All
- Fixed
deleteAll
, I've to use$iterator
and not$file
to do the dot check.Comment #14
das-peter CreditAttribution: das-peter commentedRe-rolled.
Comment #15
das-peter CreditAttribution: das-peter commentedRefactoring.
Comment #16
smiletrl CreditAttribution: smiletrl commentedReproduce steps
Comment #17
aspilicious CreditAttribution: aspilicious commentedI'm confused did the errors dissapear after applying this patch? (will test it tonight)
Comment #18
smiletrl CreditAttribution: smiletrl commented@das-peter
Tested your latest patch, and this warning disappeared. Nice job!
Comment #19
marcingy CreditAttribution: marcingy commentedThis works for me on windows as well. Reroll as htaccess changes already seem to have gone in so patch does not apply cleanly.
Comment #20
sunLooks great and seems to perform required fixes.
I'm seeing these PHP warnings a dozen of times each day.
Comment #21
Dries CreditAttribution: Dries commentedCommitted to 8.x. Thanks.
Comment #23
sunRelated (Windows): #1885796: PHP warnings in PhpStorage\FileStorage::save()