Remove the cache_key_with_version method

[bogdan]

Summary

Remove the cache_key_with_version method

ActiveSupport::Cache now use 2 conventional methods:
cache_key and cache_version (instead of 3)

The concatenation of key with version is now performed inside
AS::Cache.expand_cache_key instead of cache_key_with_version

r? @georgeclaghorn

[georgeclaghorn]

Does this mean ETags generated by Action Controller won’t include the version? Are we missing a test for that?

[bogdan]

Yes, are right. That thing is probably not covered. We should add one first.
I reviewed this whole new cache versioning structure once again.

Something tells me that cache_key_with_version is a bad idea.
It makes it confusing when to use cache_key and cache_version and when cache_key_with_version. And what should be the priority if both of them defined.

I think the concept of merging the cache_key with cache_version should be hold inside Cache.expand_cache_key.

I would propose the following changes:

• Make method accept version option def expand_cache_key(object, namespace = nil, version: false)
• Implement the [cache_key, cache_version].join("-") inside this method.
• Remove cache_key_with_version

In this way, we hold the concept of version and key separately and merge them only when explicitly specified.

[georgeclaghorn]

Adding a flag to expand_cache_key sounds like a good idea. I think it should be true by default to avoid a breaking change to the method’s default behavior—normalize_key can just set it to false.

I don’t think cache_key_with_version needs to go away, though. At the very least, it doesn’t need to go away in order to resolve the duplication here, so we can consider it separately.

[bogdan]

@georgeclaghorn I've updated the PR with the removal of cache_key_with_version method. I think this is the best thing we can do before rails release: cleanups can wait.

[georgeclaghorn]

Sorry, but this isn’t an improvement. A record is in the best position to determine whether it has a version and combine the version with its key.

I don’t find the distinction between cache_key, cache_version, and cache_key_with_version particularly confusing—certainly not to the degree that it can’t be clarified with documentation.

I’m still willing to accept a PR that implements the version flag for expand_cache_key.