Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

The last submitted patch failed testing.

drewish’s picture

Status: Needs work » Needs review
FileSize
7.99 KB

fixing a bug in the tests and a missed replacement of simpletest_clean_temporary_directory() with file_unmanaged_delete_recursive().

sun’s picture

Straightforward. Only very minor issues:

+  // Catch all for other types of files like sockets.

...I had to read twice to understand.

+
+

Doubled blank lines in file.test.

If the tests will pass and those very minor issues are fixed, this is RTBC.

drewish’s picture

FileSize
8.12 KB

thanks sun, fixed those issues.

sun’s picture

We can also provide some return value. Slightly simplified that comment.

drewish’s picture

sun, i intentionally did not return a value since you'd really need to check the whole way down the tree.

sun’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
7.82 KB

Removed the return value.

But also fixed some coding-style issues. Yikes!

sun’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
7.84 KB
sun’s picture

drewish’s picture

FileSize
7.9 KB

how about this so we avoid checking is_file is_link over and over?

drewish’s picture

FileSize
9.13 KB

okay, now with return value. and better docs.

drewish’s picture

FileSize
879 bytes

what a beautiful bikeshed we've built!

[ignore this... wrong patch]

drewish’s picture

FileSize
9.13 KB

with an extra the now.

sun’s picture

Status: Needs review » Reviewed & tested by the community
webchick’s picture

Status: Reviewed & tested by the community » Fixed

Excellent work, folks. :) Committed to HEAD.

Status: Fixed » Closed (fixed)
Issue tags: -D7FileAPIWishlist, -ImageCacheInCore

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