search

FriendsOfCake / cakephp-upload

CakePHP: Handle file uploading sans ridiculous automagic
https://cakephp-upload.readthedocs.io/
MIT License
551 stars 255 forks source link

[cake-4.x] nameCallback filename is not used. #533

Closed challgren closed 4 years ago

challgren commented 4 years ago

Fixes #532 I tried to use the ProcessorInterface but this lead to the nameCallback being called multiple times. So the best option I could find was to pass the desired filename as a argument to the TransformerInterface to replicate how cakephp-upload 4.x worked.

ADmad commented 4 years ago

Personally I have always thought of the transformer to be an unnecessary extra step. If someone wants to write multiple files for a single uploaded file they can just use a custom writer instead of custom transformer.

challgren commented 4 years ago

One solution I do see is to replace $value with $filename on line 246 and 251, but that could cause issues if people are doing multiple transformations.

ADmad commented 4 years ago

but this lead to the nameCallback being called multiple times

I would expect the callback to return the same result regardless of the number of times it's called.

challgren commented 4 years ago

but this lead to the nameCallback being called multiple times

I would expect the callback to return the same result regardless of the number of times it's called.

Not if using UUIDs for filenames

ndru123 commented 4 years ago

I'm also having this issue when upgrading to cake 4.x. Would be nice if it could be merged.

challgren commented 4 years ago

Any word on merging this? My client would really love to go live with their site and this is the only package that doesn't have Cake 4.x branch thats working.

ADmad commented 4 years ago

IMO it would be better to pass the filename as an argument to transform() method instead of the constructor.

challgren commented 4 years ago

IMO it would be better to pass the filename as an argument to transform() method instead of the constructor.

Will that actually get this merged? And you could have mentioned it like over a week ago instead of telling me you are waiting for the repo owner to merge it.

ADmad commented 4 years ago

And you could have mentioned it like over a week ago instead of telling me you are waiting for the repo owner to merge it.

That's a great response to the only individual who's even interested in responding to your PR. Good luck waiting for someone else to respond / merge this.