Trashbin skip list

[AlexAndBear]

Description

With this change new config parameters has been introduced.
Admins can now decide, based on file extensions, directory names and size,
if a resource should not be observed by the trashbin and deleted immediately.

Motivation and Context

How Has This Been Tested?

• test environment:
• test case 1:
• test case 2:
• ...

Screenshots (if appropriate):

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Database schema changes (next release will require increase of minor version instead of patch)
• Breaking change (fix or feature that would cause existing functionality to change)
• Technical debt
• Tests only (no source changes)

Checklist:

• Code changes
• Unit tests added
• Acceptance tests added
• Documentation ticket raised:
• Changelog item, see TEMPLATE

[AlexAndBear]

originated from https://github.com/owncloud/files_backup/pull/39

[jvillafanez]

Just a couple of things so far:

• I'd move the checks to its own class. This way it's easier to add unit tests for this and it also keeps the code isolated.
• I'd try to avoid using regular expressions unless needed.
  • So far, the examples provided in the config.php are for extensions and files inside folders (basically, paths ending or starting with some chars). This can be done faster without regular expressions.
  • Regular expressions are error-prone. My concern here is that the admin could enter a "wrong" regular expression, such as .*.png which could affect to files the admin doesn't want to, such as a_file.apng. This simply cannot be detected.
  • Case insensitiveness seems a bad idea if it wasn't asked for explicitly. The config.php file doesn't say anything about this.
• Taking into account that we don't expect too many skips, we probably should include some logs at info level (warning might be too much) for tracing purposes.

[AlexAndBear]

@jvillafanez what do you think about the current solution?

[jvillafanez]

I still think it's better to move the checks to a dedicated class. We'll need to add unit tests, and it should be easy to add them in a dedicated class. It should also be easier to include an initialization phase to clean the config input and make the actual action easier to read.
In addition, it might be better to split into multiple smaller functions in order to give meaningful names to the functions to make the code easier to follow.

Also note that there could be filenames with "*" in the path (I don't think we're doing anything special), so there could be a "*abc" folder in the root.

[AlexAndBear]

@jvillafanez No Thing, we can move it

[jvillafanez]

Note that we should include an option to skip the trashbin by filesize, which will probably be the main use case for this feature. Taking this into account, maybe having "trashbin.skip.extensions", "trashbin.skip.directories" and "trashbin.skip.sizes" as config options might be a better choice

[jvillafanez]

I think so. The code seems complex at first sight, probably because the PHP string functions aren't easy to handle, and it also has some edge cases we need to take care. I don't mind having the wildcard logic if it easy to follow though.
My concern is that a 3rd person might take 1-2 days trying to put a fix there successfully because every time he touches something he breaks another thing. If you're confident your code is fool-proof, go ahead.

[AlexAndBear]

@jvillafanez can you have a second look, this is not quite final, tests need to be added.

[jvillafanez]

For the config.sample file, I prefer to use the default values if any. If it isn't possible (the defaults in this case will likely be empty lists, which doesn't add any value), some sane defaults is a better choice, so it's easy to copy and paste them. You might also want to clarify whether there is a default value in use or not (mainly to clarify that 1GB files will still reach the trashbin by default, for example). You can add additional examples in the comments to clarify things.
In this regard, you could include:

• for the extensions, the .exe files or .mkv might be more common choices.
• for the directories, I think our use case is the .snapshot folder. It's something we might need, so better to have it around
• for the filesize, 1GB seems reasonable. Depending on the usage, you might want to increase it to 5GB or 10GB

[JammingBen]

@jvillafanez I updated the PR with some general improvements and your suggestions. Also added tests for the new class. I'm not a 100% sure about the ´config.sample´ yet. At least it should now be more clear how to use these config params.

Thanks for your help here btw 👍

[jvillafanez]

Warning messages seem harsh. I understand that we might want that message for the "skip directories" configuration as a deterrent, but the ones for the extension and file size... I expect the skip by file size to be widely used.

[AlexAndBear]

TBD:

• Inform @mmattel to run config-to-docs-command for 10.8

[mmattel]

@janackermann @jvillafanez
Could you pls make in config.php each first line a brief description line as you can see in the other examples above or below.
The first line of a new config parameter is always taken as line for the table of contents. The lines below can be anything you like. See also the doc on the web how it renders.
I would like to avoid post merge corrections if possible 😃

[jvillafanez]

I think those will be the last changes for me.

[mmattel]

For some strange reasons, already committed text suggestions for config.php got corrupted.
For one smaller corruption, I changed the bad line and committed it directly.
The other reconstructed suggestion is multi line and I would like to get another eye on @janackermann

EDIT: fixed

[mmattel]

@jvillafanez

the .htaccess file shouldn't be modified.

This has been fixed by @janackermann, no changes in the file.
Pls re-review/approve.

[mmattel]

OK from the config.php POV
Code not tested

[jvillafanez]

Tiny detail, maybe for the next time.

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
6 Code Smells

No Coverage information
No Duplication information