Maybe we can make this configurable? Valid values are the results of hash_algos().

Maybe we should just allow people to implement a key generator interface and inject that into the constructor and have it do createCacheKey from a message. If you're ok with that, I can make that abstraction now.

Hm, okay, but I would be happier if we could make it kind of optional: add a hash option in config defaulting to sha1, allowed values are null and result of hash_algos. (createCacheKey in null case should throw a LogicException if no hash and no CacheKeyGenerator instances are provided). Also, a custom CacheKeyGenerator (name ok?) that could be provided via a setter (with a HashCacheKeyGenerator implementation used as the default).

IMO in most cases such implementation might be an overkill, so I think it would be better to provide a "fallback"/default solution.

WDYT?

Why would anyone want to customize this? If they do, a CacheKeyGenerator would be fine?

PS I really dislike setters. I prefer things to be immutable after instantiation.

PS I really dislike setters. I prefer things to be immutable after instantiation.

Me too, but considering that this adds an extra configuration effort, this seems to be a half-way solution to me.

Also, changing the constructor would be a BC break (unless the generator is optional?), while adding an extra config option and an optionally used setter is not.

I was thinking making it optional, with a default value of null, then filling it using a ternary statement, yeh.

Hmm, just looked at the constructor signature. I think using the options resolver might be better.

Hmm, just looked at the constructor signature. I think using the options resolver might be better.

Hmm, right. You can still fall back to a string then using the hash implementation?

I've implemented the hash algo selection. How does that look?

Can you please add change log? We follow keep a change log format.

Wanted to avoid conflicts with other PRs. Do you mind adding something on merge?

Thank you. 👍

Looks good to me, thanks @GrahamCampbell

I'm a little bit late here but this PR is a BC break in a minor version. All my caches are become broken due to the new hash algo used for the cache key generation.

@sagikazarmark Can you confirm it has been done by mistake?

Anyway, as the new tag has been released, let it as it is, I just need to change my cache config.

I don't think it would be considered broken to just loose items from the cache?

did you get errors, or just cache misses? i think cache misses on a minor version sound ok. i hope we did not release it as a patch release though, that would be unexpected.

I agree, cache misses doesn't sound like BC break.

Yes, I probaby abuse when saying BC break as technically everything is working fine (just differently)... I use it in a specific context: caching http requests done to a third service secured by IPs and use the generated cache for tests execution on Travis but after upgrading, my cache is becomes broken. As already said, not a big deal to fix, just want to discuss your BC policy.

That is an interesting question. Our general BC promise is here: http://php-http.readthedocs.io/en/latest/httplug/backwards-compatibility.html

But should we consider the cache keys as a part of the BC promise? Whatever we decide should be documented.

I suggest that we are allowed to update the cache keys for minor versions but not for patches. But I would try to avoid it.

For the record:
This PR also updated the cache keys: #8 (merged and released)
And the PR for this issue will likely update the cache keys: #4

I would say the following :

Do not make our keys part of our BC promise, but consider them as it was the case.
So we can make changes to the key in a minor or patch version, but we should try our best to avoid that.

Actually this specific PR could even be a bugfix: with massive usage it's possible that some requests collide using md5.