enable file_unmanaged_delete() to handle stream wrapper URIs [#573292]

Comment	File	Size	Author
#12	dont-resolve-streamwrapper-before-delete.patch	958 bytes	jbrown

#3	file-inc-file_unmanaged_delete-recursive--handle-stream-wrappers.patch	1.58 KB	eMPee584

	file-inc-file_unmanaged_delete-handle-stream-wrappers.patch	816 bytes	eMPee584

Comment #1

eMPee584 CreditAttribution: eMPee584 commented 15 September 2009 at 10:49

Issue tags:

+Usability, +API clean-up

(tagging)

Log in or register to post comments

Comment #2

sun

German

Karlsruhe

CreditAttribution: sun commented 30 September 2009 at 17:50

The same also for file_unmanaged_delete_recursive(), please.

Log in or register to post comments

Comment #3

eMPee584 CreditAttribution: eMPee584 commented 1 October 2009 at 11:15

File	Size
file-inc-file_unmanaged_delete-recursive--handle-stream-wrappers.patch	1.58 KB

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

Log in or register to post comments

Comment #4

sun

German

Karlsruhe

CreditAttribution: sun commented 1 October 2009 at 12:49

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

Log in or register to post comments

Comment #5

eMPee584 CreditAttribution: eMPee584 commented 1 October 2009 at 16:04

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.

Log in or register to post comments

Comment #6

sun

German

Karlsruhe

CreditAttribution: sun commented 1 October 2009 at 18:01

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.

Log in or register to post comments

Comment #7

eMPee584 CreditAttribution: eMPee584 commented 1 October 2009 at 21:47

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

Log in or register to post comments

Comment #8

drewish CreditAttribution: drewish commented 4 October 2009 at 19:24

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

Log in or register to post comments

Comment #9

sun

German

Karlsruhe

CreditAttribution: sun commented 9 October 2009 at 13:01

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.

Log in or register to post comments

Comment #10

webchick

she/they

English

Vancouver 🇨🇦

CreditAttribution: webchick commented 9 October 2009 at 18:39

Status:

Reviewed & tested by the community

» Fixed

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

Log in or register to post comments

Comment #11

23 October 2009 at 18:40

Status:

Fixed

» Closed (fixed)

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

Log in or register to post comments

Comment #12

jbrown CreditAttribution: jbrown commented 13 May 2011 at 21:31

Version:	7.x-dev	» 8.x-dev
Category:	task	» bug
Status:	Closed (fixed)	» Needs review

File	Size
dont-resolve-streamwrapper-before-delete.patch	958 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().