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

add ResolverInterface::isStored #3

Conversation

havvg
Copy link

@havvg havvg commented Dec 16, 2013

see liip#284

*/
public function isStored($path, $filter)
{
return $this->getResolver($filter)->isStored($path, $filter);
Copy link

Choose a reason for hiding this comment

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

@havvg do we really need such methods in the manager?

<?php
$cacheManager->getResolver($filter)->isStored($path, $filter);

Maybe something like this would be better. I am just wonder.

Copy link
Author

Choose a reason for hiding this comment

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

I just added this method because the other forwards are also present.

Copy link

Choose a reason for hiding this comment

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

we can back to this later. good for now.

@makasim
Copy link

makasim commented Dec 16, 2013

I think we should throw an exception when resolve method is called before store one. I can do it after merging your. Just confirm you think we should have an exception.

@havvg
Copy link
Author

havvg commented Dec 16, 2013

What do you mean by "when resolve method is called before store", why is that an exceptional state/process?

@makasim
Copy link

makasim commented Dec 16, 2013

let's assume one tries to execute next code

<?php

$cacheManager->isStored($path, $filter);
//false

//and now I am trying to call resolve
$cacheManager->resolve($path, $filter);

//what should it return? I propse to throw exception in this case

@havvg
Copy link
Author

havvg commented Dec 16, 2013

The resolve method is only responsible for returning an URL to read the image from. The only case I can see, where such an exception is valid, would be a resolver which needs to connect to a datasource to ask for the respective URL where the path is the identifier.

However by design there is no need for this. I'm fine with adding an exception class to be thrown, so one exists within the namespace and we can handle those. The bundled resolver are fine as they are. They are capable of resolving to the target url, even if there actually might be no image available.

@makasim
Copy link

makasim commented Dec 16, 2013

yeah the bundle itself would never throw this exception. Because we use the logic in the right order isStored if not store and then resolve. But let's assume one implement its own logic and somehow called resole before actually storing something to cache. The exception is needed I think

@havvg
Copy link
Author

havvg commented Dec 16, 2013

The exception itself is fine and should be provided by the bundle. But no resolver of this bundle is throwing it.

You don't know what the user wants to accomplish when he calls resolve on a non-stored image. It's not an exception as it, you may call resolve just to get the url to be used in some output, and actually store/sync/.. the image later on, e.g. upon KernelEvents::TERMINATE, because you are creating a huge load of filtered images after uploading the source image.

The resolver of this bundle are just fine. They work by interface and contract. If one implements its own logic non-contractual, it's the business of the user to handle unwanted side-effects with the components in use.

@makasim
Copy link

makasim commented Dec 16, 2013

thanks for the good explanation. Now I now more about possible use cases of the bundle

makasim added a commit that referenced this pull request Dec 16, 2013
@makasim makasim merged commit b5e14a5 into formapro-forks:resolver-get-rid-of-get-browser-path Dec 16, 2013
makasim pushed a commit that referenced this pull request May 25, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants