Comments (13)
OK, that was made before I realized how this really works :).
I still don't like involving events for something they shouldn't supposed to do, but it has to be like that for now. I'll fix this as soon as I find time for it.
from cakephp-file-storage.
Why do you think an event is not the right tool here? I would like to get some arguments that convince me to not use an event here. Before you write code let's discuss your alternative solution.
Everything else that does the actual processing is implemented in the listener, so following SoC there should be a single central place that is separated from other that deals with this. And that place is the listener for me.
The path building is bound to the requirements of the adapter. The adapter config and storage backend (S3 vs Local for example) matters as well for what path builder is used and which settings. All of this is determined in the listener. It seems to be pretty straight forward and logical to me to use the listener.
Even if you would like to get the path builder instance inside the behaviour you'll have to somehow to get the instance of the event listener that will process the current entity. You need it because the listener knows the correct path builder configuration. I don't think there is any good way to get the path builder from the listener other than using an event that would return it. And doing that we can instead directly return the built path from the listener using an event.
from cakephp-file-storage.
For the next major release I'd move all the file and image processing code to the seperate classes that aren't tightly coupled to the event system. A listener isn't a good place to do the procesing. A listener could have some FileProcessor passed as a dependency and act as a proxy - call the right methods at a right time. The point is to have the code that can be flexible and reusable in different context not having to depend on an external system - that is the events in our example. Events in CakePHP are an implementation of the observer pattern. So they should observe events triggered in an application and handle them. Placing the file and image processing code inside them is way out of they concern. Which in my opinion breaks the SoC principle.
I'll be very busy for the next few weeks. But after that I'll think about some better architecture and present it to you.
from cakephp-file-storage.
I'll be very busy for the next few weeks. But after that I'll think about some better architecture and present it to you.
That's fine, but let's make a quick decision to not delay the release for weeks. Will my proposed change work for your architecture plans? I dislike the idea of introducing something new here and then dropping it again in a few month. This should be build in a way it works in the future as well without any changes at all if possible.
I'm OK with your proposal so far but this won't remove the need to fire an event because of the reason I've explained before. The only difference will be that we're going to remove logic from the "proxy" event into separate classes as you suggested.
from cakephp-file-storage.
My suggestions are more like for 2.0 (or even 3.0) version of the plugin. Let's not delay anything. If you have the time, you can reimplement those two methods like you suggested - I'm ok with that for 1.x versions. This plugin is now based on listeners heavily and I don't want to change anything in that aspect for now.
from cakephp-file-storage.
But even if they're for that, I don't like the idea of introducing things and throwing them away within a few month again. This is pretty bad for the users of the plugin. So I would like to implement this in a way that would provide a flawless and if possible completely BC compatible future upgrade path.
from cakephp-file-storage.
I'll be out and travel for the next 2 days and probably won't have internet access, so don't expect to hear from me until I'm back on Friday.
from cakephp-file-storage.
@robertpustulka I know you're busy but could we please make a decision on how to implement this? I would like to go ahead with 1.1.0.
from cakephp-file-storage.
The event based approach you suggested is currently the best one. I'm leaving for a week tomorrow so I won't be able to do it myself in that time.
from cakephp-file-storage.
I'll take care of it, enjoy your vacations!
from cakephp-file-storage.
Thanks, I'll think about the rest of the issues after I come back :)
from cakephp-file-storage.
@robertpustulka the way I implemented it now doesn't require a helper, instead you can now get (any) path directly from the entity. I'm just still not happy with the image versions but I think this should be handled in a similar way. Maybe I'm going to add FileStorageEntity::versionPath('type', $options); or something similar, I guess I'll have to try a few things to figure out a nice way to use that functionality.
I think the version path and filename should be handled by the path builders as well but then they need to become aware of the image processing.
from cakephp-file-storage.
Closing this in favour of #94
from cakephp-file-storage.
Related Issues (20)
- No versions of jpeg files HOT 4
- Keep a Change Log HOT 2
- Can't save file when creating related model HOT 2
- Upgrading from v1 to v2 HOT 7
- Tag current 2.0 version with next semver tag HOT 1
- How can one add to the list of supported adapter classes? HOT 5
- No image manipulation or correct url paths being returned HOT 2
- updating docs HOT 3
- Todo: Add migration guide from Cake 3 to 4 HOT 2
- updating docs
- Class could be found HOT 4
- Followed the guidelines in documentations server complaints 'Burzum\FileStorage\Event\LocalFileStorageListener' not found HOT 3
- Created a script to upload files to S3 - files uploaded but script throws error The listener `Burzum\FileStorage\Storage\Listener\LocalListener` doesn't allow the `\Gaufrette\Adapter\AwsS3` adapter class! Probably because it can't work with it HOT 4
- cakephp/cakephp 4.0.x dependency issue HOT 7
- Version 4.0 (complete rewrite) HOT 3
- Antipattern showcase
- Link in docs is broken
- Issues with upload for 1:1 HOT 1
- Issues installing via composer HOT 8
- Looking for a new maintainer
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from cakephp-file-storage.