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

[1.0][resolver] Decouple WebPathResolver from http request. Simplify its logic. #320

Merged

Conversation

makasim
Copy link
Collaborator

@makasim makasim commented Feb 6, 2014

This PR introduce next features:

  • WebPathResolver and NoCacheWebPathResolver no more coupled to request. Could be executed in cli. You just have to configure parameters used by request context as default values.
  • AbstractFilesystemResolver was removed.
  • NoCacheResolver was renamed to NoCacheWebPathResolver since could only work with local images. I think previous name would confuse someone.
  • WebPathResolverTest are units and does not use real filesystem any more.
  • WebPathResolver relly on filesystem services everywhere.
  • CreateCacheDirectoriesCompilerPass was removed as it seems filesystem does its job very well (I tested it in different scenarios).

@makasim
Copy link
Collaborator Author

makasim commented Feb 6, 2014

@havvg ping

@@ -23,7 +23,7 @@ function isStored($path, $filter);
* @param string $path The path where the original file is expected to be.
* @param string $filter The name of the imagine filter in effect.
*
* @return string The URL of the cached image.
* @return string The ABSOLUTE URL of the cached image.
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't yell :-)

@havvg
Copy link
Contributor

havvg commented Feb 7, 2014

NoCacheResolver was renamed to NoCacheWebPathResolver since could only work with local images. I think previous name would confuse someone.

The new name is misleading. The resolver has nothing to do with the web path (even if the previous implementation extended the web path resolver). It's more a NoopResolver (doing nothing).

@havvg havvg added this to the v1.0.0 milestone Feb 7, 2014
@makasim
Copy link
Collaborator Author

makasim commented Feb 7, 2014

The new name is misleading.

disagree, it expects that given path is the correct path inside the web dir. Which is not correct for aws keys or files stored in the db. path could be anything.

@makasim
Copy link
Collaborator Author

makasim commented Feb 7, 2014

@havvg fix ABSOLUTE and test name, could we address provider maybe later?

@havvg
Copy link
Contributor

havvg commented Feb 7, 2014

Yeah, let's postpone this one for now :)

havvg added a commit that referenced this pull request Feb 7, 2014
…e-from-request

[1.0][resolver] Decouple WebPathResolver from http request. Simplify its logic.
@havvg havvg merged commit d8ec507 into liip:develop Feb 7, 2014
@makasim
Copy link
Collaborator Author

makasim commented Feb 7, 2014

thanks! we are moving forward fast 👯

@makasim makasim deleted the web-path-resolver-decouple-from-request branch February 7, 2014 08:04
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