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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Hydra’s picture

Status: Needs review » Reviewed & tested by the community

Since this is exactly the purpose of drupal_unlink, this should be adjusted as suggested from das-peter. Thanks for the patch!

chx’s picture

Title: MTimeProtectedFastFileStorage should use drupal_unlink() » Remove file.inc ties and fix unlike while at it in MTimeProtectedFastFileStorage
Status: Reviewed & tested by the community » Needs work

Thanks 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.

das-peter’s picture

Added a protected method MTimeProtectedFastFileStorage::unlink() with the same code as drupal_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.inc
However I'm not sure how to solve that, shall we duplicate more code? :|

das-peter’s picture

Status: Needs work » Needs review
FileSize
6.77 KB

Next version after a talk in IRC with aspiliciious & chx.

[14:24] <chx> das-peter: can you get rid of the recursive delete call?
[14:30] <chx> das-peter: so http://privatepaste.com/801d135116 this is how you can do a recursive delete but i am not sure how to make this work w/ the chmod you need to do :(
...
[14:34] <chx> das-peter: it's in FileStorage
[14:34] <chx> das-peter: deleteAll
[14:34] <chx> das-peter: nevermind, then, let's do it in a separate issue (but if you feel up to it, please please do!)

I changed quite a bit now (besides the coding standard fixes):

  • I've added a method FileStorage::unlink(), this basically does the same as drupal_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 in file_unmanaged_delete_recursive(). However, I'm not sure if the watchdog part is necessary.
  • MTimeProtectedFastFileStorage::saveHtaccess() is the replacement for file_save_htaccess(), I stripped all the stuff I thought is unrelated.
das-peter’s picture

IRC Log:

[15:38] <chx> das-peter: isnt that a fatal?
[15:38] <chx> das-peter: public static function filePreDeleteCallback($path) {
[15:39] <chx> das-peter: $this->filePreDeleteCallback($this->directory);
[15:39] <chx> das-peter: static::filePreDeleteCallback($this->directory); would be my guess
[15:39] <das-peter> chx: What the heck?! I tested that and got no errors. That's not good and anyway, I don't think it needs to be static anymore...

...

[15:49] <chx> das-peter: oh and watchdog , get rid of that
[15:49] <chx> das-peter: nothing Drupal-y please.

Done as said.

Status: Needs review » Needs work

The last submitted patch, MTimeProtectedFastFileStorage-1849570-5.patch, failed testing.

das-peter’s picture

Status: Needs work » Needs review
FileSize
1.04 KB
6.45 KB

Added check if dir is a dot.

Status: Needs review » Needs work

The last submitted patch, MTimeProtectedFastFileStorage-1849570-7.patch, failed testing.

chx’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, MTimeProtectedFastFileStorage-1849570-7.patch, failed testing.

chx’s picture

That'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.

das-peter’s picture

Title: Remove file.inc ties and fix unlike while at it in MTimeProtectedFastFileStorage » Remove file.inc ties and fix unlink while at it in MTimeProtectedFastFileStorage
Status: Needs work » Needs review
FileSize
3.04 KB
6.01 KB

Next 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.

Status: Needs review » Needs work

The last submitted patch, MTimeProtectedFastFileStorage-1849570-12.patch, failed testing.

das-peter’s picture

Status: Needs work » Needs review
FileSize
2.28 KB
6.56 KB

Re-rolled.

[12:52] <das-peter> chx: Hi, looks like you were right about the permissions in FileStorage::deleteAll(), I could add another utility class which extends RecursiveIteratorIterator to set the permissions before accessing / deleting the child items.
[12:52] <chx> das-peter: i thought that will be needed :/ is that better than a recursive php function?
[12:54] <das-peter> chx: I'm not sure if it's better. Just thought you like the RecursiveIteratorIterator and thus I should keep that ;)
[12:55] <chx> das-peter: skip the iterators
[12:56] <chx> das-peter: PHP 5.4 has http://www.php.net/manual/en/class.recursivecallbackfilteriterator.php but we cant use , skip em
[12:56] <chx> das-peter: just use a recursive method done
[12:56] <das-peter> chx, okay, so I'll clone the existing one and strip what ever possible.
[13:07] <chx> das-peter: yeah
[13:08] <chx> das-peter: check my htaccess code, that's how far you can get
[13:08] <chx> das-peter: those file.inc methods, boy, those are complicated
[13:09] <das-peter> chx: do you've another patcht? I thought I've implemented the latest version you made.
[13:09] <chx> ah yes
[13:09] <chx> you did
[13:09] <chx> good :)
das-peter’s picture

Refactoring.

[13:33] <chx> das-peter: cos unlink and deleteRecursive call only fieldPreDeleteCallback and so the latter can be inline into the code in deleteRecursive , you decide what to call it, i think the parent class has an unlink, no? so unlink to overrde parent might be better? really, your choice
smiletrl’s picture

FileSize
48.2 KB

Reproduce steps

  1. Pick a windows machine.
  2. Install drupal 8 in default language 'English', using the standard profile.
  3. Install Language module. You get unlink warning.
aspilicious’s picture

I'm confused did the errors dissapear after applying this patch? (will test it tonight)

smiletrl’s picture

@das-peter
Tested your latest patch, and this warning disappeared. Nice job!

marcingy’s picture

This works for me on windows as well. Reroll as htaccess changes already seem to have gone in so patch does not apply cleanly.

sun’s picture

Priority: Minor » Normal
Status: Needs review » Reviewed & tested by the community
Issue tags: +Windows

Looks great and seems to perform required fixes.

I'm seeing these PHP warnings a dozen of times each day.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks.

Status: Fixed » Closed (fixed)

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

sun’s picture