fullFilePath() & fileUrl() is implemented wrong in the event-overhaul branch

[burzum]

@robertpustulka this won't work.

    public function fullFilePath(EntityInterface $entity, array $options = []) {
        $pathBuilder = $this->createPathBuilder($entity['adapter']);
        return $pathBuilder->fullPath($entity, $options);
    }

    public function fileUrl(EntityInterface $entity, array $options = []) {
        $pathBuilder = $this->createPathBuilder($entity['adapter']);
        return $pathBuilder->url($entity, $options);
    }

The path builder won't match the adapter name! For example you have configured Local1 and Local2 or S3BucketFiles and `S3BucketImages - I guess you get the idea - there are no such path builders (S3BucketFilesPathBuilder...) available.

I would really prefer to trigger an event here so that all logic that is involved for an adapter is handled inside the related listener.

Here is my proposed change:

    public function fullFilePath(EntityInterface $entity, array $options = []) {
        $event = $this->dispatchEvent('FileStorage.fullFilePath', [
            'record' => $entity
        ]);
        return $event->result;
    }

    public function fileUrl(EntityInterface $entity, array $options = []) {
        $event = $this->dispatchEvent('FileStorage.fileUrl', [
            'record' => $entity
        ]);
        return $event->result;
    }

[robertpustulka]

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.

[burzum]

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.

[robertpustulka]

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.

[burzum]

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.

[robertpustulka]

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.

[burzum]

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.

[burzum]

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.

[burzum]

@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.

[robertpustulka]

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.

[burzum]

I'll take care of it, enjoy your vacations!

[robertpustulka]

Thanks, I'll think about the rest of the issues after I come back :)

[burzum]

@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.

[burzum]

Closing this in favour of #94