Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Finishes out the 'october:util purge uploads' placeholder function #4029

Closed
wants to merge 4 commits into from

Conversation

austinderrick
Copy link
Contributor

Finishes out the october:util purge uploads placeholder function.

@LukeTowers
Copy link
Contributor

@w20k ready for testing

@LukeTowers
Copy link
Contributor

@bennothommo is this something you'd be interested in testing / reviewing?


$this->info('Purged: '. basename($file));
$totalCount++;
@unlink($file);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@austinderrick Could you check if the file is writable and can be deleted before saying it is purged and trying to unlink it, and indicate to the user if it could not be deleted? It is currently saying it has purged files that may not have actually been removed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, if you don't mind, could you also make the same change to the utilPurgeThumbs() method? :)

@bennothommo
Copy link
Contributor

@austinderrick @LukeTowers Works well, although I did notice that it does not delete folders that have been emptied, so it can potentially leave a lot of orphaned directories.

@LukeTowers
Copy link
Contributor

@austinderrick @bennothommo could we use deleteEmptyDirectory() on the File model to clean up empty directories: https://github.com/octobercms/library/blob/master/src/Database/Attach/File.php#L779?

@w20k
Copy link
Contributor

w20k commented Jul 4, 2019

Works great - confirmed ✅

@LukeTowers
Copy link
Contributor

@w20k We still need @bennothommo and @austinderrick to respond to my last question

@w20k
Copy link
Contributor

w20k commented Jul 4, 2019

@LukeTowers just had a few hours of spare time to run through all my todos or half ;)

@bennothommo
Copy link
Contributor

bennothommo commented Jul 4, 2019

@LukeTowers I've assigned the PR to myself to have a look at the method you suggested, as it appears @austinderrick has abandoned it. Hopefully will give it another look in the next couple of days.

@bennothommo bennothommo added this to the v1.0.458 milestone Jul 8, 2019
@LukeTowers LukeTowers modified the milestones: v1.0.458, v1.0.459 Jul 22, 2019
bennothommo added a commit that referenced this pull request Aug 4, 2019
Credit to @austinderrick for the initial implementation (#4029)
@bennothommo
Copy link
Contributor

This PR is now being finalised in #4518.

@bennothommo bennothommo closed this Aug 4, 2019
@bennothommo bennothommo removed this from the v1.0.459 milestone Aug 11, 2019
LukeTowers added a commit that referenced this pull request Aug 15, 2020
Only works for uploads stored on the local disk right now, support for remote disks may be added in the future at some point.

Replaces #4518 & #4029. Credit to @LukeTowers, @bennothommo, & @austinderrick.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants