The current implementation of SkipDotsRecursiveDirectoryIterator doesn't work on systems where dotfiles are included in the list, and the first entry is a dotfile. Next is only called when iterating to the next element, rewind is triggered to reset the iterator to the first element.

I triggered this bug on a php 5.4.11 system.

The fix is simple, skip the dot files either on next or on rewind calls.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

openminds’s picture

Version: 7.9 » 7.19
openminds’s picture

PatchRanger’s picture

Version: 7.19 » 7.x-dev
Status: Active » Needs review

I assume you meant that it needs review.

openminds’s picture

I don't know - it works for me, so it should somehow work its way through all the drupal-how-to-get-a-patch-in-core...

bfroehle’s picture

Okay yes. This looks good. Do we need to advance in __construct() too? Or does that call rewind?

Krizalys’s picture

According to the PHP documentation, rewind() is automatically called when an Iterator instance is being iterated, as demonstrated in this example. Combined to the patch proposed, this behavior would guarantee that dots are always skipped while iterating.

bfroehle’s picture

Status: Needs review » Needs work

@Krizalys: Yes, but I just read through the PHP source and their implementation (at the C level) would end up calling the equivalent of our skipdots in __construct, next, and rewind.

It's a little pedantic, but I think we should also call skipdots in __construct. This would fix the result of creating an instance and immediately calling current.

compa’s picture

Just applied this to a 7.20 as well (un PHP 5.4.11) - confirmed it fixed my modules-upgrade issue

Krizalys’s picture

Priority: Normal » Major

I'm raising the priority of this issue since Drupal 7 will face the same bug (not being able to upgrade some modules, among other things, will be one consequence) in PHP 5.3.23, the next version to be released. Ideally, this patch should be applied to Drupal 7.21.

bfroehle’s picture

Status: Needs work » Needs review
FileSize
662 bytes
Christopher Riley’s picture

Thank you for the patch it worked perfectly and kept me from going bald by pulling my hair out.

Niklas Fiekas’s picture

Issue tags: +Needs tests

Awesome, thanks!

Since this is not needed for D8 the patch is probably good to go. Is there a good way to catch the problem in an automated test?

hansfn’s picture

Status: Needs review » Reviewed & tested by the community

The patch in comment #10 is good. RTBC-ing.

mjross’s picture

I applied the patch in comment #10, and it didn't resolve the module update problem in a clean install (Drupal 7.21 and PHP 5.4.11). I'd be happy try any other proposed solutions or alternate patches, and report the results.

hansfn’s picture

Are you 110% sure that you did the test correctly - cleared cache and all that before testing? What exactly was the error message you got?

mjross’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs manual testing

Yes, I verified that the patch changes were applied to /includes/filetransfer/filetransfer.inc, and cleared the cache. The error message is the type listed at that issue I linked to. For instance, for the Token module:

    Error installing / updating
    File Transfer failed, reason: Cannot remove directory /sites/all/modules/token\..
hansfn’s picture

OK, I don't have any other suggestions.

PS! Why is the slash backward in stead of forward. On what OS are you testing.

mjross’s picture

Windows 7. I haven't examined the Drupal code in detail, but presumably Drupal uses the standard "/" in the directory path, and when it seeks the parent directory, Windows returns "\..".

Downgrading the version of PHP might be a final option, but I would much rather avoid that.

David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs manual testing

It sounds like this needs more manual testing, especially if we can't write automated tests for it (and if there are no existing ones)? Especially to make sure it doesn't cause any regressions in the module install/update functionality.

The code looks pretty reasonable though.

David_Rothstein’s picture

So basically, if people can report that they've tested actual Drupal functionality that uses this code, and make sure it continues to work correctly, we should be able to get it committed.

Yazzbe’s picture

#10 is working for me too. D 7.21 installed.

David_Rothstein’s picture

@yazzy008 (and others who commented earlier): It would be useful if you could summarize in more detail what steps you actually took to test the patch :) Thanks!

millionleaves’s picture

I encountered an error running module updates from within Drupal.

This comment summarises the iterations I went through to fix the issue: http://drupal.org/node/1900580#comment-7209694

Essentially, I'm running PHP 5.4 and Drupal 7.21; the patch referred to in #10 resolved the issue I was experiencing with module updates. I have not tested it any more than that.

mjross’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs manual testing

I repeated the module update process, and it worked this time. It is baffling that different results occurred even though no changes were made to the files. The only apparent change from yesterday vs. today is the system reboot (this morning), which of course means Apache was restarted; but I can't imagine how that would affect the results. Anyway, the patch seems fine. Thank you to everyone who worked on it.

Anonymous’s picture

I ran into this bug today and applying the patch at #10 fixed that module update error on 7.21. I did not try downgrading PHP (currently 5.4). Thanks a lot!

anomalophobe’s picture

Ran into the module update problem on a fresh install of Drupal 7.21 (PHP 5.4), and #10 fixed my issue - thanks much! Will report back if I have any further issues once I configure the site.

arne@dbyte.nl’s picture

Patch at #10 also fixed the problem for me (drupal 7.19 + PHP 5.3.23)

greggmarshall’s picture

Patch at #10 worked for me Drupal 7.21 + PHP 5.3.23 on a Linux server

hansfn’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs manual testing

I think we have gotten enough manually tests now. The only negative report was positive on the second try.

David_Rothstein’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +7.22 release blocker

Thanks for the extra testing, and the patch!

Committed to 7.x. http://drupalcode.org/project/drupal.git/commit/d9cf661

This issue still has the "needs tests" tag... if anyone can figure out how to write automated tests for it, it would be great to reopen to do so.

David_Rothstein’s picture

Oops, wrong issue tag.

briflo’s picture

This patch fixed the update problem for me. Xammp on Windows. Drupal 7.21 PHP 5.4.7

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

toni.mueller’s picture

Hello,

how can I apply this patch? I have the issue "File Transfer failed, reason: Cannot remove directory".

Best regards,

Toni