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

Option to delete file on record deletion #362

Merged
merged 10 commits into from
Mar 18, 2016
Merged

Option to delete file on record deletion #362

merged 10 commits into from
Mar 18, 2016

Conversation

jorisvaesen
Copy link
Collaborator

No description provided.

Verified

This commit was signed with the committer’s verified signature.
ctavan Christoph Tavan
@josegonzalez
Copy link
Member

Just fyi, I only merge changes that do not decrease test coverage (including PHPCS!) and have the requisite docs. Happy to work towards that though :)

Joris Vaesen added 3 commits February 17, 2016 22:04

Verified

This commit was signed with the committer’s verified signature.
ctavan Christoph Tavan
phpcs fix and docs update

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@jorisvaesen jorisvaesen mentioned this pull request Feb 17, 2016
* @param array $files the files being written out
* @return array array of results
*/
public function delete(array $files)
Copy link
Member

Choose a reason for hiding this comment

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

You need a test for this method as well.

@josegonzalez
Copy link
Member

This works now?

josegonzalez and others added 2 commits March 12, 2016 15:36
Rather than mocking the DefaultWriter, we mock the responses from the filesystem to say that a delete has succeeded or failed. This will suffice for testing the proper responses from DefaultWriter::delete()
Mock underlying fileystem writes
@josegonzalez
Copy link
Member

@jorisvaesen does this work according to how you'd like it now?

@jorisvaesen
Copy link
Collaborator Author

I did some testing and it seems to work as expected.

josegonzalez added a commit that referenced this pull request Mar 18, 2016
Option to delete file on record deletion
@josegonzalez josegonzalez merged commit b652f1c into FriendsOfCake:master Mar 18, 2016
@josegonzalez
Copy link
Member

Thanks for being patient!

@josegonzalez
Copy link
Member

@jorisvaesen added you as a maintainer. Don't abuse your new role of course! I'm looking forward to your help in maintaining this plugin, especially considering the nice features you've worked on :)

@kinglouie
Copy link

maybe it is a silly question but how can I delete additionally generated thumbnails with this feature? I thought I would just overwrite the delete method in the writerinterface but the passed array $files only has the path and not the filename, so I cant reference my thumbnail from there.

@kinglouie
Copy link

I just did a quick and dirty solution to show the issue.

MyOwnFileWriter.php

public function delete(array $files)
{

    $filesystem = $this->getFilesystem($this->field, $this->settings);
    $results = [];
    $results[] = $this->deletePath($filesystem, $this->entity['file_dir'] . $this->entity['file']);
    $results[] = $this->deletePath($filesystem, $this->entity['file_dir'] . 'thumbnail-' . $this->entity['file']);

    return $results;
}

my solution obviously doesn't work when you have multiple files per entity but my MyOwnFileWriter is specific for my database so in my case there wont be an issue.

@josegonzalez
Copy link
Member

@KingLoui since we cannot tell what your original urls will be - that isn't stored - your method is more or less what I would do.

public function delete(array $files)
{
    $files[] = $this->entity['file_dir'] . 'thumbnail-' . $this->entity['file']
    return parent::delete($files);
}

The above might be better.

@jorisvaesen
Copy link
Collaborator Author

For automating this, I was thinking to use the transformer.

Create an extra function, let's say beforeTransform() where thumbnails can be created.
And let transform() only return the array of files which can later be used by the writer to delete all files (including thumbnails).

@josegonzalez
Copy link
Member

Maybe, though I'd have to see implementation/docs first. Also, requiring this in the interface would be a BC break, just fyi.

@jorisvaesen
Copy link
Collaborator Author

Indeed.
For 3.x the writers delete function should be overridden (like you said before) but maybe it could be an option for 4.x ?

@josegonzalez
Copy link
Member

Sure.

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.

None yet

3 participants