We discovered this in our pre-production release... xmlsitemap_uninstall() will eat your files directory. The ordering of the code in the hook will delete the configuration containing the location of the files directory, then attempt to delete the xmlsitemap subdirectory. It ends up running _xmlsitemap_delete_recursive('public', TRUE).

From looking at the code, it looks as though Drupal 7 may also be susceptible to this bug, although I didn't confirm that.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

rbayliss created an issue. See original summary.

rbayliss’s picture

This patch is probably the simplest, safest step, which is just to not delete any directories during uninstall.

rbayliss’s picture

Title: Deletes entire files directory on uninstall » Uninstall hook is capable of deleting entire files directory under some configurations
Priority: Critical » Major

Reclassifying this, since I've discovered that this is at least partly on our end. Someone had deleted the xmlsitemap.settings config in advance of the uninstall. I'd still argue that the recursive delete is risky with a configurable path, but this no longer seems critical.

moshe weitzman’s picture

I'll also point out that xmlsitemap_uninstall() never calls config->save() so its attempt to cleanup its config is fruitless.

DanielVeza’s picture

Can confirm this is still happening. Just had it happen to one of our sites. This is pretty critical. Needs to be committed ASAP.

DanielVeza’s picture

Priority: Major » Critical

I think I would classify this as Critical considering the potential impact that it can have.

Dave Reid’s picture

I'll also point out that xmlsitemap_uninstall() never calls config->save() so its attempt to cleanup its config is fruitless.

I'm not really sure what this means. Config should be cleaned up automatically by core if it's owned by the module being uninstalled?

Dave Reid’s picture

Rather than avoiding the cleanup, let's make it safer. If the directory is FALSE, this should abort the deletion.

Dave Reid’s picture

I see what moshe meant, I just ended up removing the section in xmlsitemap_uninstall() that "cleared" the config, because it's not necessary to do.

Status: Needs review » Needs work

The last submitted patch, 8: 2987939-safely-delete-directory.patch, failed testing. View results

Dave Reid’s picture

The last submitted patch, 11: 2987939-tests-only.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Dave Reid’s picture

Looks like PHPUnit 6+ added the assertDirectoryExists() methods. I want to ensure the tests pass on 8.5 if possible.

Status: Needs review » Needs work

The last submitted patch, 13: 2987939-safely-delete-directory.patch, failed testing. View results

Dave Reid’s picture

Try that again.

Dave Reid’s picture

Version: 8.x-1.x-dev » 7.x-2.x-dev
Status: Needs review » Patch (to be ported)

I'm satisfied with the test coverage here, committing #15 to 8.x-1.x. I will backport this to 7.x-2.x.

Dave Reid’s picture

Patch for 7.x-2.x.

  • Dave Reid committed c392f9a on 8.x-1.x
    Issue #2987939 by Dave Reid, rbayliss: Fixed edge case scenario where...

Status: Needs review » Needs work

The last submitted patch, 17: 2987939-tests-only.patch, failed testing. View results

Dave Reid’s picture

Try that again without short array syntax.

Status: Needs review » Needs work

The last submitted patch, 20: 2987939-tests-only.patch, failed testing. View results

  • Dave Reid committed 3cab415 on 8.x-1.x
    Follow-up #2987939: Fixed @covers errors while running DirectoryTest.
    

  • Dave Reid committed c67137a on 7.x-2.x
    Issue #2987939 by Dave Reid, rbayliss: Fixed edge case scenario where...
Dave Reid’s picture

Version: 7.x-2.x-dev » 8.x-1.x-dev
Status: Needs work » Fixed

Committed #20 to 7.x-2.x.

Status: Fixed » Closed (fixed)

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

Heine’s picture

We just lost a files directory to this during a deployment (currently restoring from backup). A new alpha release with this fix would be appreciated.

As to #3, config is removed (in memory) prior to the delete by xmlsitemap_uninstall.