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

Update WebPathResolverFactory.php #467

Merged
merged 3 commits into from
Jul 30, 2014
Merged

Update WebPathResolverFactory.php #467

merged 3 commits into from
Jul 30, 2014

Conversation

JJK801
Copy link
Contributor

@JJK801 JJK801 commented Jul 29, 2014

Hi,

I'm actually updating CmfMediaBundle, i get an error while running test:

Could not find configuration for a filter: resolve

It run well when i make this change.

Is something wrong in my configuration or is it a bug?

@dbu
Copy link
Member

dbu commented Jul 29, 2014

one test is failing

@JJK801
Copy link
Contributor Author

JJK801 commented Jul 29, 2014

Yes, i just wanted tout open the debate about it

@dbu
Copy link
Member

dbu commented Jul 29, 2014

ah sorry, now i see what you are doing. but i am not familiar enough with this, hope @makasim can give us some input here.

@makasim
Copy link
Collaborator

makasim commented Jul 30, 2014

There was a bug #429. In few words Safari rejects 301 redirect if they looks same as the current url. So for the first time (when you generate the cache) you would not see images in Safari. The fix 80eb36a just makes thouse urls (one to generate cache and the cache result) look differently.

@JJK801
Copy link
Contributor Author

JJK801 commented Jul 30, 2014

@makasim Thanks for the explaination, but i'm still wondering why i get an error, i think there must be a bug by using WebPathResolver as it directly call the /media/cache/resolve url (which must be called only after cache were generated as far i undertand), and it throws my error as in the routing file, URL scheme is /media/cache/{filter}/{path}.

@makasim
Copy link
Collaborator

makasim commented Jul 30, 2014

/media/cache/resolve/{filter}/{path} - the url of the action which generates the cache. called only when you do not have the cache.
/media/cache/{filter}/{path} - where web path resolver stores cached image.

@JJK801
Copy link
Contributor Author

JJK801 commented Jul 30, 2014

@makasim CacheManager told me the inverse:

public function getBrowserPath($path, $filter, array $runtimeConfig = array())
{
        ...

        return $this->isStored($path, $filter) ?
            $this->resolve($path, $filter) :
            $this->generateUrl($path, $filter)
        ;
}

and routing too:

<route id="liip_imagine_filter" path="/media/cache/{filter}/{path}" methods="GET">
        <default key="_controller">%liip_imagine.controller.filter_action%</default>
        <requirement key="filter">[A-z0-9_\-]*</requirement>
        <requirement key="path">.+</requirement>
</route>

I think i understand what you wanted to do, but the implémentation seems wrong.

IF       the image is not stored in cache
THEN generate url with /media/cache/resolve/{imagePath}
THEN hit the image controller
THEN generate the image cache
THEN redirect to /media/cache/{imagePath}

IF       the image is stored
THEN generate url with /media/cache/{imagePath}
THEN hit the file directly

am i right?
if yes i just have to update the routing file and invert the getBrowserPath condition and it will work.

@JJK801
Copy link
Contributor Author

JJK801 commented Jul 30, 2014

@dbu @makasim is this update ok for you? (cf my last commit)

@@ -42,7 +42,7 @@ public function addConfiguration(ArrayNodeDefinition $builder)
$builder
->children()
->scalarNode('web_root')->defaultValue('%kernel.root_dir%/../web')->cannotBeEmpty()->end()
->scalarNode('cache_prefix')->defaultValue('media/cache/resolve')->cannotBeEmpty()->end()
->scalarNode('cache_prefix')->defaultValue('media/cache')->cannotBeEmpty()->end()
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is not ok, as after this change we would again have problems with Safari

Copy link
Collaborator

Choose a reason for hiding this comment

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

ah, I see, igonre the comment

@makasim
Copy link
Collaborator

makasim commented Jul 30, 2014

So you changed urls used to generate and the actual url of cached image?
Now its like

/media/cache/resolve/{filter}/{path} - the url of the action which generates the cache. called only when > you do not have the cache.
/media/cache/{filter}/{path} - where web path resolver stores cached image.

Could you confirm?

@JJK801
Copy link
Contributor Author

JJK801 commented Jul 30, 2014

@makasim yes, thats it :) IMO it's prettiest to store in the /media/cache folder (resolve folder means nothing) and use the /media/cache/resolve path to execute the resolution.

@makasim
Copy link
Collaborator

makasim commented Jul 30, 2014

@JJK801 yes that was my intention too but I think I messed it up.

merging!

thanks.

makasim added a commit that referenced this pull request Jul 30, 2014
Update WebPathResolverFactory.php
@makasim makasim merged commit a85ef19 into liip:master Jul 30, 2014
@JJK801
Copy link
Contributor Author

JJK801 commented Jul 30, 2014

@makasim @dbu thanks guys 👍

@JJK801 JJK801 deleted the patch-1 branch July 30, 2014 14:49
@makasim
Copy link
Collaborator

makasim commented Jul 30, 2014

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.

4 participants