Problem/Motivation

The module provides an option to enable Trash support on the File entity, but I believe the File entity will require some special handling in order for it to work in a trash-like manner.

The idea being that moving a file to the trash should ideally also hide the file from the public, e.g. by moving them to a private://trash_files/ folder or something until they're restored.

There are some modules that do something similar we can take inspiration from:

Proposed resolution

If it's decided that the additional complexity involved in trying to support might not be worth the hassle.

We can either explicitly disable the File entity from being supported, or try to support "soft-deletions" of the underlying file.

Remaining tasks

N/A

Issue fork trash-3583917

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

codebymikey created an issue. See original summary.

codebymikey’s picture

codebymikey commented
Issue summary: View changes

amateescu made their first commit to this issue’s fork.

amateescu opened merge request !124

amateescu’s picture

amateescu commented
Status: Active » Needs review

There are a couple of problems with moving to the private:// stream wrapper:

- it might not be configured on all sites
- we don't know whether the file was using the private scheme before being trashed (unless we encode the scheme in the new file name)

But I think we can implement a "good enough" solution that doesn't involve private://, just move the file to an unguessable location in the public scheme.

codebymikey’s picture

codebymikey commented

I have to say, I'm extremely impressed with the solution here!

I was under the impression it'd be a bit of a goliath task to achieve, but moving it into a hidden .trashed subdirectory makes perfect sense without introducing too much complexity.

During my test, ran into an issue with compatibility with the path_alias alias integration, so I've pushed the code up for that for you to review.

There was also a tiny bug relating to how dirname() works with URIs for top-level files and it resolving to a directory called public: in the root of the project and failing to write to it (the tests only passed because it created the initial public: folder as part of the setup), so updated the code to use the file_system service to resolve them instead.

The final commit is mainly for edge-case handling (relating to me being unable to restore a file after I ran into the path_alias crash), but if you don't feel it's necessary to keep, feel free to remove it.

  • amateescu committed 6ed4b458 on 3.1.x 
    task: #3583917 Special support for File entities

By: amateescu
By:...

  • amateescu committed 41bea2e1 on 3.x 
    task: #3583917 Special support for File entities

By: amateescu
By:...
amateescu’s picture

amateescu commented
Status: Needs review » Fixed

Your changes look great, thanks! Made a few adjustments and merged to 3.1.x and 3.x.

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

codebymikey’s picture

codebymikey commented
Status: Fixed » Active

There's one more bug I ran into while testing the File entity integration.

It seems there's still other parts of the code (in particular the EntityRestoreForm) calling \Drupal\Core\Entity\EntityInterface::toUrl() without checking that the entity supports it. So adding a fix and functional test and will mark for Review shortly.

One other Drupal quirk I just realized is that if you call /file/$fid/delete without a destination query string, it'll result in a crash due to how \Drupal\Core\Form\ConfirmFormHelper::buildCancelLink()/code> works.

codebymikey’s picture

codebymikey commented
Status: Active » Fixed

The issue was actually related to the latest version of the #3376216: Translation support patch, since it includes a link to the entity during restorations.

Will push the fix there instead.

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.