Refactoring 1.0.0

[havvg]

Hi there,

I would like to start working on refactoring some parts of this bundle.
Those parts may not be backwards compatible, therefore the major version will be increased according to http://semver.org.

First of all, I would like to remove the targetPath (see #132, #135) from the user land.
This will make sure, the user always interacts with the bundle (e.g. on the CacheManager) using the same filename, currently called path. The current call to resolve should not be done by the user himself.

In additional I want to remove the dependency of Request within the ResolverInterface. It's not actually required (see AmazonS3Resolver). The request should be injected into those resolver, requiring it (e.g. by the CacheManager using a respective interface); see #142.

Furthermore I would like to move forward to Psr\Log\LoggerInterface replacing Symfony\Component\HttpKernel\Log\LoggerInterface.

As there are Resolver that issue requests or have other meaningful workload there should be a caching mechanism for the ResolverInterface::resolve method (see #156).

What do you think? What's missing?

[lsmith77]

sounds like a good plan. I will tweet this ticket to get more feedback

[Maxwell2022]

Upgrading the aws sdk to the last version? https://github.com/aws/aws-sdk-php

[stof]

Moving to PSR-3 requires bumping the requirement to Sf 2.2 (or injecting a wrapper in 2.1 rather than the Sf logger)

[mahono]

Don't know if I should create a separate issue: I noticed that when you set cache_prefix to an empty string, cache:clear will completely delete the contents of the web folder. So it would be useful to have a security check here to prevent people from losing data accidently. (I "solved" it by setting cache_clearer: false, but only the backup saved my life..)

[stof]

@mahono you should create a separate issue to report the bug IMO

[havvg]

@mahono That's indeed an issue, to be fixed asap.

[havvg]

@Maxwell2022 Yes, this would be a neet feature. I would like to post-pone it and not make it a requirement for the new release.

@stof True, the dependency would change, which is fine by me. The wrapper should be part of the bundle.

[dbu]

+1 in moving to sf 2.2 for 1.0 to have PSR-3 logging. version 0.9 works and people who don't update symfony probably also don't update this bundle...

[dbu]

this definitely needs more love. to invalidate a phpcr-odm cached image, i ended up with this

    $filter = 'image_upload_thumbnail';
    $path = $this->manager->resolve($this->container->get('request'),
                                                     $document->getId(), 
                                                     'image_upload_thumbnail'
                                                     )->getTargetUrl();
    $path = substr($path, strpos($path, $filter) + strlen($filter));
    $this->manager->remove($path, $filter);

the resolver is not guaranteed to return a string, it can also be a RedirectResponse. and then the remove expects the path but without the prefix of imagine and filter name that the resolver just added to the path.

[havvg]

That's exactly what's the issue with path and targetPath and the Request part, which will be removed. Goal is $this->manager->remove($document->getId(), $filter) and considering #161 $filter should be an array of filters and optional, too.

Anyway this line $path = substr($path, strpos($path, $filter) + strlen($filter)); should not be required in 0.9.x.

[marcospassos]

@havvg Consider $this->manager->remove($document->getId()) that removes all cached filters.

[dbu]

as far as i understood, that is the idea here.

and considering #161 $filter should be an array of filters and optional, too.

[makasim]

I am planing to rework clear\remove methods of CacheResolver. As it seems to be the last issue that have not been addressed yet here.

You can remove a cache of single filter if string is passed or a set of filters in case of array. This is how the remove method will look like:

<?php

/**
 * @param string         $path
 * @param array|string $filters
 */
function remove($path, $filters);

After thinking about clear method for a while I decided to remove it. As it really hard and I'd say dangers to implement it for resolvers like amazon s3.

WDYT?

[dbu]

i think we should also have something that allows me to clear all configured filters of one image. actually thats even the standard case to me unless you happen to know what filters are used with your image - which is very hard unless you hardcode and then you forget that you started using a different filter with the same images and then you don't invalidate properly anymore.

btw, having an event when images actually get deleted would be very cool, for example to integrate with varnish.

[makasim]

it could be possible if get all currently configred filters names and pass them as second arguments. @dbu is it sounds ok to you? We can also introduce a method in CacheManager which takes filter names and do remove, just a helper method.

<?php

$cacheManager->removeAll($path);

btw, having an event when images actually get deleted would be very cool, for example to integrate with varnish.

event system would be indeed a good addition.

[dbu]

imho, one method could be enough, remove($path, $filters = array()) and if the filters array is empty (not passed) we collect all configured filters. but a removeAll would also work and be more explicit.

[makasim]

closing as all discussed features\refactorings etc were implemented in 1.0. reopen if you think I miss something.