-
Notifications
You must be signed in to change notification settings - Fork 379
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][filter] Dynamic filters #313
Conversation
$filterPostfix = ''; | ||
if ($runtimeFilters = $request->query->get('filters', array())) { | ||
$signer = new UriSigner('aSecret'); | ||
if (false == $signer->check($request->getRequestUri())) { |
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.
Here I want to reused UriSigner
from HttpKernel to check url was not modified
@havvg the logic is ready for review. I still have to fix some tests and add new |
@havvg added tests, updated README.md. Ready for review |
@havvg ping |
{ | ||
if (!empty($runtimeConfig)) { | ||
return $this->generateUrl($path, $filter, $runtimeConfig); |
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.
I dont like this place because it generates url of local controller
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.
@ASKozienko I dont like it neither. In future it has to be optimized, for now I think we can start with simple solution and later change implementation.
looks good in general |
@havvg ping |
1 similar comment
@havvg ping |
+1 on maintainer rights. I'm quite busy these days, and got some illness, too :( |
@havvg oh you are here (:. I wish you get better very soon. |
I have given @makasim commit rights. |
Thanks! |
@lsmith77 thanks, is there any policy I have to follow as a maintainer. just a few words about what I can and what I cannot (: to make things clear. |
Congratulations @makasim ! |
@makasim The only real policy right now: Keep the master branch stable (by following semver.org). Feel free to maintain :-) |
@havvg thanks, will do |
I am going to merge this tomorrow. (if tests pass ofcourse) |
Will the use of this logic be added to the documentation soon? |
@trsteel88 it is already there read README.md. Search for |
@trsteel88 please note you have to read the doc of |
Thanks @makasim. I just wanted to raise 1 issue with this implementation. Currently, you are generating a hash to authenticate that the values are correct. I think this should be split into several "directories". Eg, say the generated hash is "S8rrlhhQ" I think it should be split up so it becomes "S8r/rlh/hQ" (or something similar). Otherwise, if a website has 100,000 images it is going to create up to 100,000 folders inside 1 (or more) directories. Splitting it into several directories will scatter the structure a bit more. This will make it much easier to navigate through ftp or viewing the folder and doing "ls -la". |
@trsteel88 Good point. I've created an issue #341 |
if ('\\' === $pathinfo['dirname']) { | ||
$pathinfo['dirname'] = ''; | ||
} | ||
$filterUrl = $this->router->generate('_imagine_'.$filter, $params, true); |
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.
Should the $params be sorted alphabetically (by key)?
Eg, say your dynamic filter is {width: 100, height: 200, extra: {'key': 'val'}}
If I then generate a image using {height: 200, width: 100, extra: {'key': 'val'}} it will generate a different hash.
I think that these parameters should be sorted to try and minimise duplicate image generation. Obviously the params would need to cascade incase their are multiple array within the config.
Thoughts?
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.
that's a good idea, you could create an issue so we dont forget about it,
This attempts to address #268. Other solutions\issues: #42, #271
Use cases:
Here you can see filter url:
and resolved one:
The url contains
PkE1ct4Q
in path to not mess with pre configured filter caches. It is not possible to remove a single custom cache (at least you the whole path includingPkE1ct4Q
). but remove all has to work quite well.