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] do not expose targetPath #282

Merged
merged 2 commits into from
Dec 15, 2013

Conversation

makasim
Copy link
Collaborator

@makasim makasim commented Dec 10, 2013

next refactoring step after #281

fixes #162

@LouTerrailloune
Copy link
Contributor

  1. Regarding getBrowserPath, may be it should return null when no specific url is need (or when cache has not been generated), the cache manager will then return the target url (which point to the imagine controller).
  2. Regarding the injection of the cacheManger into the resolvers, is it really needed ? (seems to be used only by WebPathResolver to get the web root and by other resolvers to get the target url (which will not be needed anymore if getBrowserPath return null (first point above)).

@makasim
Copy link
Collaborator Author

makasim commented Dec 11, 2013

I've setuped a sandbox for this bundle. I am testing web path and aws s3. It gives me more confidence in the work I am doing.

Just want to let you know.

@LouTerrailloune I would respond you asap. I still have to review the code and check your suggestions.

@makasim
Copy link
Collaborator Author

makasim commented Dec 12, 2013

@havvg this is ready for review.

@LouTerrailloune thanks for your reply I found Resolver::getBrowserPath method completely useless. I am trying to get rid of it and I would open another PR once it is ready.

@@ -52,18 +52,13 @@ public function __construct(DataManager $dataManager, FilterManager $filterManag
public function filterAction(Request $request, $path, $filter)
{
$originalImagePath = $path;
Copy link
Contributor

Choose a reason for hiding this comment

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

There is no need for this variable.

@havvg
Copy link
Contributor

havvg commented Dec 13, 2013

The CacheManager::remove method should rename its parameter to path, too.

} else {
if ($this->logger) {
$this->logger->warn('The object could not be created on Amazon S3.', array(
'targetPath' => $targetPath,
'targetPath' => $objectPath,
Copy link
Contributor

Choose a reason for hiding this comment

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

The context key should be renamed to objectPath, too.

@makasim
Copy link
Collaborator Author

makasim commented Dec 13, 2013

@havvg I believe I address all your comments. What about tests? Are you succeeded to run them locally?

To run tests you have to run composer with --dev and the phpunit. nothing special.

@makasim
Copy link
Collaborator Author

makasim commented Dec 13, 2013

@havvg I did not squash commits because you may want to review them separately. Will do a rebase and squash once you say it is good to be merged

@havvg
Copy link
Contributor

havvg commented Dec 14, 2013

Thanks so far, I will take another look. The tests are running now locally.

@havvg
Copy link
Contributor

havvg commented Dec 14, 2013

PR out, good to rebase/squash :)

@makasim
Copy link
Collaborator Author

makasim commented Dec 14, 2013

@havvg rebased&squashed. I squashed your commits as a separate commit.

@makasim
Copy link
Collaborator Author

makasim commented Dec 14, 2013

@havvg ping

havvg added a commit that referenced this pull request Dec 15, 2013
…rget-path

[1.0][resolver] do not expose `targetPath`
@havvg havvg merged commit 14def51 into liip:develop Dec 15, 2013
@havvg
Copy link
Contributor

havvg commented Dec 15, 2013

Thank you!

@makasim makasim deleted the resolver-do-not-expose-target-path branch December 15, 2013 16:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Attn: Blocker This item blocks other issue(s) or PR(s) and therefore must be resolved prior.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants