Make ProviderCache not final

[cookieguru]

I'd like my cache keys to be namespaced. The easiest way for me to do so would be to simply override the getCacheKey method to work how I want and pass that to the geocoder. However that's not possible with it being final.
I don't want to reimplement every single method (read: copy and paste). Therefore I propose marking this class not final so that this method and the getName method can be overridden. I have marked the other methods as final and changed the private properties to protected.

[Nyholm]

Thank you. I think your use case makes sense.

[Nyholm]

Could you write a change log in this provider for a new minor release?

[cookieguru]

Sure, would this go under a new 4.0.1 release in CHANGELOG.md or...?

[Nyholm]

You should put it under a new heading called 4.1.0 in this file: https://github.com/geocoder-php/Geocoder/blob/master/src/Provider/Cache/CHANGELOG.md.

[cookieguru]

Done

[Nyholm]

Thanks.
But I think you updated the wrong changelog.
The correct one is: /src/Provider/Cache/CHANGELOG.md

[cookieguru]

Oops, you're right. Fixed now

[Nyholm]

Thanks

[Nyholm]

Oops. I see that we both missed to remove the final keyword on the class =)

[unkind]

IMHO, it would be better to pass 4th argument like callable $cacheKeyStrategy = null.

Inheritance is like regex. If you try to solve something with inheritance, you have 2 problems.

Think about this next time you want to remove final. Maybe it's not worth it.

Not sure if it's possible now due to BC, but I can refactor it in composition manner. ping @Nyholm

new ProviderCache($realProvider, $cache, null, function (string $query) {
    // namespace logic for keys
});

[cookieguru]

@unkind I disagree, dependency injection is not the right way to solve this. By using that method you have no access to the cache instance itself, which may itself have some properties or methods that are necessary to compute the cache key. Your method also forces the re-declaration of the method upon instantiation, which isn't always necessary or desired.

[unkind]

@cookieguru

have no access to the cache instance itself

Do you mean

new ProviderCache($realProvider, $cache, null, function (string $query) use ($cache) {
    // namespace logic for keys and you have $cache instance here
});

?

which may itself have some properties or methods that are necessary to compute the cache key

Can you provide the use case?

Your method also forces the re-declaration of the method upon instantiation

You can use DI-factories to instantiate just once.

Your method also forces the re-declaration of the method upon instantiation, which isn't always necessary or desired.

Even if it is a problem, you may use lazy loading. Your protected method in fact is lazy loaded key naming strategy. It's a weird to inherit something in order to substitute something.

Inheriting concrete classes is a red sign by itself. There are always way to solve it with composition. What if I need to decorate $cacheKeyStrategy? I have to inherit once again? Like FooProviderCache extends NamespacedProviderCache extends ProviderCache?

[cookieguru]

You can use DI-factories to instantiate just once.

What I meant is that re-declaring the method is required even if you are agnostic about its implementation.

It's a weird to inherit something in order to substitute something

I disagree. That's the whole reason why we have abstract classes.

If you look at CacheProvider it is itself just a decorator of Provider. Your criticism of overwriting this method alone seems inconsistent.

[unkind]

What I meant is that re-declaring the method is required even if you are agnostic about its implementation.

No. You can pass null for default strategy.

That's the whole reason why we have abstract classes.

ProviderCache is not abstract class and final was pointing on it very clearly.

If you look at CacheProvider it is itself just a decorator of Provider.

It is implementation of Provider. Implementing interface is fine, inheriting concrete class is a bad sign.

[cookieguru]

May I suggest this if you're unhappy with everyone else's implementation?

[unkind]

May I suggest this if you're unhappy with everyone else's implementation?

Does it mean you can't speak reasonably anymore?