[Draft] Clean interfaces for the imagine processes #1590
Conversation
|
|
||
| namespace Liip\ImagineBundle\Domain; | ||
|
|
||
| interface ImageReference |
There was a problem hiding this comment.
i renamed this to ImageReference as its the reference to an image rather than an actual image. i noticed when i got confused in LiipTransformerStackExecutor where we also have the imagine ImageInterface which holds the actual image data and has filters applied to it.
|
|
||
| use Imagine\Image\ImageInterface; | ||
|
|
||
| interface FilterExecutor |
There was a problem hiding this comment.
better name for Liip\ImagineBundle\Imagine\Filter\Loader\LoaderInterface - functionality does not need to be changed.
| * @param string $sourceImageId Identifier for the image, for example a file system path | ||
| * @param string[] $stackNames Limit invalidation to the specified stacks. If empty, all stacks are invalidated. | ||
| */ | ||
| public function invalidateCache(string $sourceImageId, array $stackNames = []): void; |
There was a problem hiding this comment.
this is not strictly about transforming, but imho makes sense to have on the main interaction point with the system.
| * @param string[] $stackNames | ||
| * @param string[] $targetFormats | ||
| */ | ||
| public function warmupCache(string $sourceImageId, array $stackNames, array $targetFormats): void; |
There was a problem hiding this comment.
i realized we said that transformToUrl should ideally not be slow and not necessarily generates the image right away but can chose to only generate it on demand. and when that is the case, we need this separate method to force warming up the cache.
There was a problem hiding this comment.
as per discussion in #1602: there is also a use case to make sure an image exists without forcing regeneration if the image already exists.
we could add a flag to this method to control regeneration:
| public function warmupCache(string $sourceImageId, array $stackNames, array $targetFormats): void; | |
| public function warmupCache(string $sourceImageId, array $stackNames, array $targetFormats, bool $regenerateExisting): void; |
on the other hand, that use case would like to get the image, so maybe a separate method public function getImage(string $sourceImageId, string $stackName, string $targetFormat): ImageReference would be more suited for that use case.
| use Liip\ImagineBundle\Domain\ImageReference; | ||
| use Liip\ImagineBundle\Imagine\Filter\PostProcessor\PostProcessorInterface; | ||
|
|
||
| class LiipTransformerStack implements TransformerStack |
There was a problem hiding this comment.
TODO: either finish this or remove it.
| use Liip\ImagineBundle\Domain\PostProcessor\PostProcessor; | ||
| use Liip\ImagineBundle\Imagine\Filter\FilterConfiguration; | ||
|
|
||
| class LiipTransformerStackExecutor implements TransformerStackExecutor |
There was a problem hiding this comment.
this is both the container for the filters and post processors and the executor for them.
| } | ||
|
|
||
| $image = $this->applyFilters($image, $config); | ||
| $resultImageReference = $this->exportConfiguredImageBinary($sourceImageReference, $image, $config); |
There was a problem hiding this comment.
we need this step before we apply post processors. and it also depends on the stack configuration. so i think we need all 3 things in the same place and can not separate them.
|
|
||
| private function exportConfiguredImageBinary(ImageReference $imageReference, ImageInterface $image, array $config): ImageReference | ||
| { | ||
| // TODO: this is for now literal copy-paste from FilterManager |
There was a problem hiding this comment.
please challenge the things below, but i don't know if we can do much better. i'd not change the configuration if we don't really need to.
| try { | ||
| $filteredString = $image->get($filteredFormat, $options); | ||
| } catch (\Exception $exception) { | ||
| // TODO: why only a problem for webp but not png/jpg? and should the stack handle this |
There was a problem hiding this comment.
we could also throw an exception (possibly containing the alternate gif image)
the problem with this is that we don't return the requested format. but from the comment below it seems we can't easily tell in advance if we do an animated gif...
|
|
||
| $this->destroyImage($image); | ||
|
|
||
| return new ImageReferenceInstance( // TODO |
There was a problem hiding this comment.
need to implement some in-memory image
| private function sanitizeFilters(array $filters): array | ||
| { | ||
| // TODO: is this much better than something like? | ||
| // if ($missing = array_diff(array_keys($filters), array_keys($this->filters))) { throw new \InvalidArgumentException('missing'.implode(',', $missing)); } |
There was a problem hiding this comment.
is the below code just legacy or is it that much more efficient?
| * | ||
| * (e.g. the URL points to a controller or a 404 handler is used to generate the image on demand) | ||
| */ | ||
| public function supportsOnDemandCreation(): bool; |
There was a problem hiding this comment.
i added this to handle both scenarios of having to generate the image when requesting the url or capable of generating on demand.
we could do a separate interface for it (but would need the method anyways because depending on the store this option could depend on whether configuration is set up)
Draft for clean interfaces as discussed at the Hackday at Liip.
We added the new interfaces and implementations under
Domainto see what is new, but should move them to the root folder when things are ready.Things that are missing in the draft
Confignamespace somehow?Will be done separatly in the
refactormain branch after merging this branch: