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

deleteCallback to delete different versions of a file #454

Merged
merged 4 commits into from
Jun 22, 2017
Merged

deleteCallback to delete different versions of a file #454

merged 4 commits into from
Jun 22, 2017

Conversation

jorisvaesen
Copy link
Collaborator

@jorisvaesen jorisvaesen commented Jun 20, 2017

Ref #453

I'll add an example to the docs if this feature gets approved.

@jorisvaesen jorisvaesen changed the title deleteCallback to automatically delete different versions of a file deleteCallback to delete different versions of a file Jun 20, 2017
- Available arguments:

- ``string $path``: Basepath of the file you want to delete
- ``object $entity``: The entity you want to delete
Copy link
Member

Choose a reason for hiding this comment

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

I would actually specify 'Entity' instead of 'object' here, just to be clearer.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Indeed.

@@ -28,7 +28,8 @@ public function setup()
'error' => UPLOAD_ERR_OK,
'size' => 1,
'type' => 'text',
'keepFilesOnDelete' => false
'keepFilesOnDelete' => false,
'deleteCallback' => false
Copy link
Member

Choose a reason for hiding this comment

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

My personal preference would be null here to demonstrate a 'nothing' value, rather than a boolean.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just saw nameCallback also uses null as default, will change this accordingly.


$behavior->config($this->dataOk);

// expecting getPathProcessor to be called with right arguments for dataOk
Copy link
Member

Choose a reason for hiding this comment

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

Not sure about committing comments to the repo, but the repo code style will be up to @josegonzalez

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Might not be a big deal as it's in a test. Although I do wonder if you have to comment each assert, does that mean they could be different test cases? Just a thought.

@davidyell
Copy link
Member

davidyell commented Jun 21, 2017

I would also probably throw in a test case for when deleteCallback is false or null just to ensure it's not executed, which might help prevent future regressions.

Otherwise, reading the code, it seems okay to me. Perhaps just a quick check with PHPCS if you've not already done that.

@josegonzalez
Copy link
Member

Seems fine, needs an example.

@saeideng
Copy link
Collaborator

nice return

                     return [
                            $path . $entity->{$field},
                           $path . 'thumbnail-' . $entity->{$field}
                       ];

good job 👍

@jorisvaesen jorisvaesen merged commit f86b5ab into FriendsOfCake:master Jun 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants