-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Serializer] Include the format in the cache key #19352
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
{ | ||
try { | ||
return md5(serialize($context)); | ||
return $format.'-'.md5(serialize($context)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be inside the md5?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's already a string and it makes the key more explicit. On the other hand if the format name is very long, it can hit the max key size. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer inside the md5: not format change for keys, shorters keys.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
{ | ||
try { | ||
return md5(serialize($context)); | ||
return md5(serialize($format.$context)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That does not work as $context is an array, so you would always have Array
and a PHP notice for the array to string conversion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad, my intent was to but it before the serialize
call ^^.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
Thank you @dunglas. |
This PR was squashed before being merged into the 3.1 branch (closes #19352). Discussion ---------- [Serializer] Include the format in the cache key | Q | A | ------------- | --- | Branch? | 3.1 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | n/a | License | MIT | Doc PR | n/a Attributes to normalize can vary by format. For instance in API Platform normalized attributes aren't the same in JSON-LD (all attributes) and HAL (relations are not serialized in the document body). This PR fixes that. Commits ------- 282eb9c [Serializer] Include the format in the cache key
Attributes to normalize can vary by format. For instance in API Platform normalized attributes aren't the same in JSON-LD (all attributes) and HAL (relations are not serialized in the document body).
This PR fixes that.