Deleting files should use the stored base path if available. #410
Conversation
|
After having a further look at this, I realize that this test isn't any good, as it creates the failure itself in a way that possible changes for the problem may not be respected. Creating a proper failing tests likely depends on how this problem is going to be fixed. I'll have a closer look later on unless someone beats me to it, may take a little as I have to get used to this amount of mocking first. |
|
Updated the PR. This should fix the problem by reintroducing the old behavior of using the path set in the entity, while keeping the possibility to use dynamic path generation. |
|
I have fixed the testsuite in #446. Please rebase. |
|
Looks good to me, nice work! |
docs/examples.rst
Outdated
| <?php echo $this->Form->input('username'); ?> | ||
| <?php echo $this->Form->input('photo', ['type' => 'file']); ?> | ||
| <?php echo $this->Form->input('photo_dir', ['type' => 'hidden']); ?> | ||
| <?php echo $this->Form->end(); ?> |
There was a problem hiding this comment.
We probably want to keep the above?
There was a problem hiding this comment.
I removed it because the photo_dir input field is not required for deleting files, and because users should not be able to modify that value, and without that field the form is the same as in the previous example.
I'd probably suggest to remove even more, and show only the different behavior configuration instead of the whole UsersTable class. But that's all personal preference, not sure what's best for the average reader. If you think it's better to provide a complete set of snippets, then I'll add it back in (without the photo_dir field).
There was a problem hiding this comment.
I want to keep examples there because users can be easily confused as to what exactly they need to write, and then think its a bug in the plugin when they just need an example vs instructions.
Feel free to remove that one field though.
|
@ADmad want to comment on the PR itself before merge? |
|
@ADmad can you have a look, if it's ok for you we can release a new version. |
|
LGTM, the merged conflicts need to be fixed. |
The mocking caused the actual getter method not to be invoked, as the magic getter method had been mocked. Also the test needs to actually make the mocked methods to return something, otherwise the field value will be `null`.
Reintroduces the usage of the stored path on delete (#390). The stored path is preferred over the dynamically generated path, as the latter might be different to the path on disk, hence causing files to be not deletable. The stored path is only used in case it's not `null`. This should ensure backwards compatibility for applications that do not store the path, and expect the path processor to be used. refs #409
Made it more clear when and why storing the path in the record is required in order for files to be deletable. Removed the form example as there is no need to pass on the path, neither should users be able to modify that value, nor is the form required in the context of deleting records/files. Also changed the form example to use proper arguments, and follow the more popular usage of not providing the base table name for the inputs.
`any()` surely doesn't suffice, as it doesn't require the method to be called at all.
|
@davidyell any thoughts here? |
|
Seems good to me, I like the small incremental improvement over introducing a whole new feature. It builds the base for a bigger PR, perhaps on a major release to work towards optionally storing the |
refs #409