Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#20 | 2987939-tests-only.patch | 2.47 KB | Dave Reid |
#20 | 2987939-safely-delete-directory.patch | 4.14 KB | Dave Reid |
| |||
#15 | 2987939-safely-delete-directory.patch | 7.15 KB | Dave Reid |
#13 | 2987939-safely-delete-directory.patch | 7.13 KB | Dave Reid |
#11 | 2987939-safely-delete-directory.patch | 6.39 KB | Dave Reid |
Comments
Comment #2
rbayliss CreditAttribution: rbayliss at Last Call Media for Commonwealth of Massachusetts commentedThis patch is probably the simplest, safest step, which is just to not delete any directories during uninstall.
Comment #3
rbayliss CreditAttribution: rbayliss at Last Call Media for Commonwealth of Massachusetts commentedReclassifying 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.
Comment #4
moshe weitzman CreditAttribution: moshe weitzman as a volunteer commentedI'll also point out that xmlsitemap_uninstall() never calls config->save() so its attempt to cleanup its config is fruitless.
Comment #5
DanielVezaCan confirm this is still happening. Just had it happen to one of our sites. This is pretty critical. Needs to be committed ASAP.
Comment #6
DanielVezaI think I would classify this as Critical considering the potential impact that it can have.
Comment #7
Dave ReidI'm not really sure what this means. Config should be cleaned up automatically by core if it's owned by the module being uninstalled?
Comment #8
Dave ReidRather than avoiding the cleanup, let's make it safer. If the directory is FALSE, this should abort the deletion.
Comment #9
Dave ReidI 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.
Comment #11
Dave ReidOk let's get some test coverage on this!
Comment #13
Dave ReidLooks like PHPUnit 6+ added the assertDirectoryExists() methods. I want to ensure the tests pass on 8.5 if possible.
Comment #15
Dave ReidTry that again.
Comment #16
Dave ReidI'm satisfied with the test coverage here, committing #15 to 8.x-1.x. I will backport this to 7.x-2.x.
Comment #17
Dave ReidPatch for 7.x-2.x.
Comment #20
Dave ReidTry that again without short array syntax.
Comment #24
Dave ReidCommitted #20 to 7.x-2.x.
Comment #26
Heine CreditAttribution: Heine at LimoenGroen commentedWe 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.