-
Notifications
You must be signed in to change notification settings - Fork 378
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] remove request parameter #278
[1.0][resolver] remove request parameter #278
Conversation
There other changes I am currently working on (would not included in this PR):
|
@@ -16,7 +16,7 @@ before_script: | |||
- composer require symfony/framework-bundle:${SYMFONY_VERSION} --prefer-source | |||
- composer install --dev --prefer-source | |||
|
|||
script: phpunit --coverage-text | |||
script: ./bin/phpunit --coverage-text |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is not necessary, please revert
@lsmith77 reverted |
{ | ||
$this->setBasePath($request->getBaseUrl()); | ||
$this->setBasePath($this->container->get('request')->getBaseUrl()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really bad, imho. I would suggest a RequestAwareInterface
and inject the Request
if accordingly. There is no reason to inject the container just to receive the base url.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RequestAwareInterface
what is this? Is shipped with symfony. Does symfony 2.0 have it?
P.S. I know that this is really bad but as far as I know this how it could be done for symfony 2.0.
* master: (25 commits) add doctrine/cache to require-dev add test on cyrillic filename for FileSystemLoader Update filters.md Add comment for image parameter in watermark filter configuration example Update doc Fix typo Add upscale filter Set ContentType of AWS cache object deprecate the phpcr loader as CmfMediaBundle provides a better one now. fix missing filename in exception Corrected aws-sdk-php link add AwsS3Resolver for new SDK version Add simple documentation Watermark loader Update dependency 'imagine/imagine' to 0.5.* fix E_NOTICE "Undefined index: mode" small fixes on Thumbnail docs added documentation on inset and outbound modes of thumbnail filter Documentation (issue liip#207) also test SF 2.3 and PHP 5.5 Add link filter ...
Could you send this PR against the develop branch, please? I just updated the branch, make sure to update your local one before sending the PR (simply rebase on the upstream one). |
close in favor #281 |
I've tried to remove
$request
parameter fromResolverInterface
as it was discussed here #155.I did not have a change to test it in real life though tests pass.