[DDC-3761] Fixed cache key

[Ragazzo]

Related with this one

[doctrinebot]

Hello,

thank you for creating this pull request. I have automatically opened an issue
on our Jira Bug Tracker for you. See the issue link:

http://www.doctrine-project.org/jira/browse/DDC-3771

We use Jira to track the state of pull requests and the versions they got
included in.

[guilhermeblanco]

@Ragazzo makes sense to me, however I do want @FabioBatSilva to review it too.

[Ragazzo]

@guilhermeblanco he reviewed it in Jira task, and said that it is an issue, sure it would be great if he will see this one more time

[FabioBatSilva]

Looks good to me,

Wondering if we should give up the more readable serialization format here and just hash $entityClass + $identifiers

[Ragazzo]

@FabioBatSilva note that you can easily go out of the limit for key, if $entityClass is namespaced, than with all prefixes and so on, you can get as it was in my case key with 244 bytes length, that is almost maximum for memcached 250 bytes key length

[DHager]

@Ragazzo : That sounds like a job for individual CacheProvider implementations: To know when a given key is too big for the system they manage, and to hash or truncate the PHP-land key in a responsible way.

Hmm... if you're worried about unexpected truncation, I'd make sure that the "most unique" parts of the ID are expressed at the start of the generated key-string. That way you reduce the chances of collision when part of the end gets chopped off.

[Ragazzo]

@DHager yes, it is question of cache implementations in particular, just raised this question to avoid unnecessary bugs in future

[Ocramius]

I agree with @DHager here: the interface says string, not which kind of string. It is up to the CacheProvider to strip/replace invalid characters or sequences of characters.

[Ragazzo]

@Ocramius so you want to do this replace in memcached provider? I think that current fix is also correct, anyway to avoid fixing any other providers it is better just not to use spaces, not sure why they were used. Anyway i think it is a critical bug, and it should be fixed asap

[Ocramius]

@Ragazzo the fix fixes the symptom, not the cause, heh

[Ragazzo]

@Ocramius sure, but i dont think that replacing in memcached or other providers sounds right, if you will use or store the hash of entity key anywhere and then will try to get it without calling providers, maybe by your own implementation you can get in situation that you forgot to replace spaces like it would be done in provider. Anyway, why this situations does not have checks in cache providers, like max key length, spaces, and so on? Should separated issue be open for that to make research on what characters are valid and what not?

[Ocramius]

Anyway, why this situations does not have checks in cache providers, like max key length, spaces, and so on?

It is indeed a bug in the cache provider, or in the specific cache adapter impl.

Should separated issue be open for that to make research on what characters are valid and what not?

I think so, yes

[Ragazzo]

Ok, than what is on this PR, should i close it and open separate about memcached cache provider implementation?

[Ocramius]

@Ragazzo yes please, and thanks! :-)

[Ocramius]

Also: close after opening the other PR, or we'll lose track of it

[Ragazzo]

Also: close after opening the other PR, or we'll lose track of it

Ok

[Ragazzo]

@Ocramius i'll try to get back to this one asap on weekend, let me know if i should speed up or if it will take too long feel free to fix it as was described )

[lcobucci]

I've opened doctrine/cache#208 to keep track of this issue, so we can close this PR.