[RFC] Allow to set a base dir for Resizer to remove from hash #32
Conversation
src/Resizer.php
Outdated
| * @param ResizeCalculatorInterface|null $calculator | ||
| * @param Filesystem|null $filesystem | ||
| */ | ||
| public function __construct($cacheDir, ResizeCalculatorInterface $calculator = null, Filesystem $filesystem = null) | ||
| public function __construct($cacheDir, $baseDir = null, ResizeCalculatorInterface $calculator = null, Filesystem $filesystem = null) |
There was a problem hiding this comment.
Why did you not add it as last argument? Would save a lot of adjustments below.
src/Resizer.php
Outdated
| */ | ||
| private function createCachePath($path, ResizeCoordinatesInterface $coordinates, ResizeOptionsInterface $options) | ||
| { | ||
| $pathinfo = pathinfo($path); | ||
| $imagineOptions = $options->getImagineOptions(); | ||
| ksort($imagineOptions); | ||
|
|
||
| $hashPath = $path; |
There was a problem hiding this comment.
If there is no $baseDir the path would not be relative, wouldn’t $relPath be misleading then?
src/Resizer.php
Outdated
| */ | ||
| private function createCachePath($path, ResizeCoordinatesInterface $coordinates, ResizeOptionsInterface $options) | ||
| { | ||
| $pathinfo = pathinfo($path); | ||
| $imagineOptions = $options->getImagineOptions(); | ||
| ksort($imagineOptions); | ||
|
|
||
| $hashPath = $path; | ||
| if (null !== $this->baseDir) { | ||
| $hashPath = $this->filesystem->makePathRelative($path, $this->baseDir); |
There was a problem hiding this comment.
I think we should use Webmozart\PathUtil\Path here instead. As in src/Image.php:108
|
I’m not a fan of this solution. How about using the file size instead of the path for the hash? |
|
I don't like the solution, either. But it seems to be the only backwards compatible way, doesn't it? |
Why? Using How about making the path relative to the already available |
|
you mean relative as in |
|
yes |
|
I guess that would work… the only difference would be that shall I update the PR ? |
|
Then I think we should do it that way. Should I create a new PR, or do you want to change this one? |
.gitignore
Outdated
| @@ -1,2 +1,3 @@ | |||
| # Vendor | |||
| /vendor/ | |||
| /composer.lock | |||
There was a problem hiding this comment.
This is unrelated to this PR I think and is already fixed in the master branch.
src/Resizer.php
Outdated
| [ | ||
| Path::makeRelative($path, $this->cacheDir), | ||
| filemtime($path), | ||
| $coordinates->getHash() |
There was a problem hiding this comment.
A trailing comma is missing here I think.
|
so do we agree on the general implementation (apart from coding style and unrelated changes) ;-) |
|
Yes, I do. |
|
I will merge this one. @leofeyer do you agree? |
|
Yes, if you remove the unrelated changes and add a unit test. |
The resizer service can then be configured with
%kernel.root_dir%/..or with%contao.root_dir%in Contao 4.4.ATTENTION: this is an API change and should result in v0.4
TODO:
realpath()to convert%kernel.root_dir%/..?