[4.2] Added A "getRepository" Method To The Cache Manager

[stevenmaguire]

Currently the repository creation within the CacheManager is protected. When adding new cache drivers the implementation requires creation of specific class. It appears to me that the CacheManager is responsible for controlling Repository implementation.

Cache::extend('elasticache', function() {
    $servers = Config::get('cache.memcached');
    $elasticache = new Illuminate\Cache\ElasticacheConnector();
    $memcached = $elasticache->connect($servers);
    return new Illuminate\Cache\Repository(new Illuminate\Cache\MemcachedStore($memcached, Config::get('cache.prefix')));
});

I propose that this Repository creation process be publicly accessible for extension, such that the above use case looks more like:

Cache::extend('elasticache', function() {
    $servers = Config::get('cache.memcached');
    $elasticache = new Illuminate\Cache\ElasticacheConnector();
    $memcached = $elasticache->connect($servers);
    return Cache::getRepository(new Illuminate\Cache\MemcachedStore($memcached, Config::get('cache.prefix')));
});

The proposed solution in this pull request adds a new public getRepository method to the CacheManager class that calls the protected repository method.

[taylorotwell]

I'm trying to figure out what this is buying you... can you post a use case?

[stevenmaguire]

This package (https://github.com/maherio/graceful-cache/pull/2/files) is attempting to add functionality to the Cache Repository class by extending the Illuminate\Cache\Repository class. This extension is overriding the repository method in Illuminate\Cache\CacheManager to create an instance of this new child class.

Without this pull request, this new child class will be used for all built-in cache drivers without issue.

However, when creating a custom driver the Repository is required to be explicitly defined in the driver definition, rather than deferring to the Repository configured in the CacheManager.

Cache::extend('elasticache', function() {
    $servers = Config::get('cache.memcached');
    $elasticache = new Illuminate\Cache\ElasticacheConnector();
    $memcached = $elasticache->connect($servers);
    return new CustomVendor\CacheRepository(new Illuminate\Cache\MemcachedStore($memcached, Config::get('cache.prefix')));
});

will become

Cache::extend('elasticache', function() {
    $servers = Config::get('cache.memcached');
    $elasticache = new Illuminate\Cache\ElasticacheConnector();
    $memcached = $elasticache->connect($servers);
    return Cache::getRepository(new Illuminate\Cache\MemcachedStore($memcached, Config::get('cache.prefix')));
});

[taylorotwell]

OK I still don't get why you need this method. Your first code example seems to do exactly what you want to do without ever having to add any extra methods.

[stevenmaguire]

That is true. Before I add my final thought, I do want to share that I don't think its worth a larger debate than is already here. So, if you review this one more time and decide its not important, close the request.

The use case demonstrated here is a well documented pattern by developers hoping to extend the capabilities of the framework, which is effective at achieving the desired behavior.

However, it is at risk when compared to the built-in drivers, because those built-in drivers defer to a decision by the CacheManager when creating a Repository object.

In the example provided, it is certainly the driver implementation's decision to utilize the ElasticacheConnector and the MemcachedStore because they are specific to the driver. I don't feel that the driver should also be responsible for deciding which repository to use, if an authority is already present.

Additionally, if the framework should decide to change the Repository being used, or a third-party package hopes to change the implementation, these custom drivers would each need to be upgraded independently.

[taylorotwell]

I'm guess I'm still just entirely confused. There is already a repository method, which your added method simply calls... so, why add any method at all? Why not just use repository? Am I missing something?

[stevenmaguire]

Whereas the existing repository method has a protected visibility, and whereas public methods within the class follow a consistent code style convention, therefore I added the public method to include public access to the protected repository method and adhere to the style conventions.

[JoostK]

In this PR you actually added it as protected. But never mind, repository itself has just been made public.

[stevenmaguire]

@JoostK This PR does not change the visibility of the repository method.

What do you mean "But never mind, repository itself has just been made public."? Where do you see this?

Making the repository method public would be equally as beneficial as this PR.

[JoostK]

See bac96f4. Which is only in 5.0, come to think of it.

[stevenmaguire]

That's amazing.

[taylorotwell]

@stevenmaguire your PR added a protected method.

[taylorotwell]

I think the fact that this PR added a proected method was the source of my confusion.

[stevenmaguire]

@taylorotwell Son of a bitch! Yep, and now I feel like a monkey. Sorry. That definitely explains the confusion all around. I was intentionally trying to provide a public method for building a repository defined by the CacheManager. Making the repository method public works as well.

Are there concerns about the naming convention of public methods inside the class?