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

Feature/new custom delete fix #3190 #3252

Merged
merged 8 commits into from
Oct 24, 2023

Conversation

benjam-es
Copy link
Contributor

This adds a test, using a custom path generator which uses a shared folder (i.e. not individual /1/ folder per image)

Once the test was failing, I extracted out the 'removeFile' and 'removeAllFiles' logic into a FileRemover class using a FileRemover Factory, to work in the same way you currently swap out path generators.

I then added a new FileBasedFileRemover class, which fixes the failing test created to demonstrate #3190.

This PR allows:

  • File removal logic can be swapped out
  • Existing file removal logic is maintaned in a DefaultFileRemoval class - ensuring everything works as before
  • New optional FileBasedFileRemover class allow us to use a custom directory path generator, which does not delete other files with is

These are split into 3 commits to see each part.

Signed-off-by: Ben James <[email protected]>
@Log1x
Copy link

Log1x commented May 2, 2023

This will be great. I was extremely confused why my entire media library was disappearing... 😩

@benjam-es benjam-es requested a review from freekmurze June 4, 2023 11:54
@elmudometal
Copy link

It will take a long time for this to be solved?

@elmudometal
Copy link

Do you have any news about this feature?

@benjam-es
Copy link
Contributor Author

@freekmurze I believe the requests were addressed in April. Let me know if you need anything else.

@freekmurze
Copy link
Member

This seems pretty nice!

Could you also write docs on this?

@b-ssr
Copy link

b-ssr commented Oct 11, 2023

Any updates on this?

@benjam-es
Copy link
Contributor Author

Awaiting @freekmurze approval I believe. I am happy to write docs on it

@freekmurze
Copy link
Member

This is looking very good. Could you also add docs for it?

@benjam-es
Copy link
Contributor Author

@freekmurze I have added a new docs MD file to detail the new functionality

@benjam-es
Copy link
Contributor Author

It looks like these tests are failing due to this: #3417

@freekmurze freekmurze merged commit a639108 into spatie:main Oct 24, 2023
1 of 15 checks passed
@freekmurze
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants