Simple fix, makes this function also usable with stream wrapper URIs.

CommentFileSizeAuthor
#12 dont-resolve-streamwrapper-before-delete.patch958 bytesjbrown
#3 file-inc-file_unmanaged_delete-recursive--handle-stream-wrappers.patch1.58 KBeMPee584
file-inc-file_unmanaged_delete-handle-stream-wrappers.patch816 byteseMPee584

Comments

eMPee584’s picture

eMPee584 commented
Issue tags: +Usability, +API clean-up

(tagging)

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

The same also for file_unmanaged_delete_recursive(), please.

eMPee584’s picture

eMPee584 commented
StatusFileSize
new file-inc-file_unmanaged_delete-recursive--handle-stream-wrappers.patch1.58 KB

np but still doubtful about the last doc change, is that *really* better English ?*g

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

The recursive function invokes itself recursive - does that work with drupal_realpath()?

eMPee584’s picture

eMPee584 commented

Yes all test cases passing.. there might be a few CPU instructions to save doing

function file_unmanaged_delete_recursive($path) {
  // Resolve streamwrapper URI to local path.
  if (!is_dir($path)) {
    drupal_realpath($path);
  }
  if (is_dir($path)) {
  ...

or not, don't think that's better though.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

The more I look into the local stream wrapper classes, the more I'm unsure whether these two functions aren't properly supporting streams already (so this would be a documentation-only issue). Are you sure?

If they really do not, then I guess we just have to instantiate proper stream wrappers instead of assuming all unmanaged files are local.

See also:
- http://php.net/manual/en/wrappers.file.php
- http://php.net/manual/en/streamwrapper.unlink.php

eMPee584’s picture

eMPee584 commented

The more I look into the local stream wrapper classes, the more I'm unsure whether these two functions aren't properly supporting streams already (so this would be a documentation-only issue). Are you sure?

YES, i tried and it did not work. That's the point *g

drewish’s picture

drewish commented

i didn't apply this but the changes look sane.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +D7 API clean-up

Tagging absolutely critical clean-ups for D7. Do not touch this tag. Please either help with this or one of the other patches having this tag.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

This looks like a good change. Committed to HEAD. Thanks!

Status: Fixed » Closed (fixed)

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

jbrown’s picture

jbrown commented
Version: 7.x-dev » 8.x-dev
Category: task » bug
Status: Closed (fixed) » Needs review
StatusFileSize
new dont-resolve-streamwrapper-before-delete.patch958 bytes

This patch was incorrect - it is up to the streamwrapper to delete the underlying file.

Currently, the only way for remote streamwrappers to be able delete a file is if they return the full URI from DrupalStreamWrapperInterface::realpath().

jbrown’s picture

jbrown commented
jbrown’s picture

jbrown commented
Status: Needs review » Closed (duplicate)

#1083982: Fix support for remote streamwrappers

gisle’s picture

gisle
he/him
Primary language Norwegian Bokmål
Location Norway
commented
Issue summary: View changes
Issue tags: -D7 API clean-up

Removing non-canonical duplicate tag. See #2426171: Multiple tags similar to 'API clean-up' for background.