#28 Remove illegal characters from caching hash according to PSR6 specs

[samvdb]

See #28

[samvdb]

Well the dot is a valid character according to the RFC so its a sure bet. However i replaced the regex with strreplace for performance and removed the dot. This can cause empty hashes in some cases i think. A better approach would be hashing the json_encode string, however this might impact performance. What would you recommend?

[Bilge]

It seems there are advantages and disadvantages for each approach.

Hashing

• :white_check_mark: Key length constrained to reasonable limit with fixed length.
• :negative_squared_cross_mark: Slight performance hit.

Performance hit could be mitigated by selecting an appropriate hash implementation. We do not need a cryptographic hash because security is not a concern; we just need a hash with near-guaranteed uniqueness for different input data.

String replacement

• :white_check_mark: Faster.
• :negative_squared_cross_mark: Variable key length with no upper limit.
• :negative_squared_cross_mark: Increased chance of collision due to malformed JSON (to be verified).

Which do you prefer?

[samvdb]

Another con for hashing is that debugging will become harder because keys are not readable anymore. I will stick with str_replace

[Bilge]

Very well, however consider we may still switch to hashing in future if the need arises.

For now, let's replace the banned characters with a single character to avoid the chance of collisions (instead of removing them). It is important that it is not possible to deliberately craft a request to create the same key for different data, which should be easier to avoid if we do replacement instead of removal.

[samvdb]

I have added tests and will add the dot again. Would you mind if i changed the hash function to protected instead of private?

[Bilge]

I do not think the hash function should be protected because I do not see the need for inheritors to override it. I presume you are asking because you find it difficult to test as a private method?

[samvdb]

No i was asking because other connectors might want to extend cachingconnector and implement hashing if need be.

I'll leave it private since the issue should be resolved for the moment.

[Bilge]

This is now merged into master and tagged as 🏷 3.2.1.

Thanks @samvdb for your work 👍

[samvdb]

Thanks Bilge!