Skip to content
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

[Cache] provider does not respect option maxIdLength with versioning enabled #27752

Merged
merged 1 commit into from Jul 1, 2018
Merged

[Cache] provider does not respect option maxIdLength with versioning enabled #27752

merged 1 commit into from Jul 1, 2018

Conversation

kshtompel
Copy link

@kshtompel kshtompel commented Jun 27, 2018

Q A
Branch? 3.4
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #27746
License MIT
Doc PR

Component symfony/cache generates cache item ID longer then maxIdLength when versioning is enabled

@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Jun 27, 2018
@nicolas-grekas nicolas-grekas changed the title Ticket 27746 Cache provider does not respect option maxIdLength with … [Cache] provider does not respect option maxIdLength with versioning enabled Jun 27, 2018
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for spotting and fixing. Ok to me with a minor comment.
If possible could you add a test? It might be hard so don't spend hours on it...

@@ -255,7 +255,8 @@ private function getId($key)
return $this->namespace.$this->namespaceVersion.$key;
}
if (\strlen($id = $this->namespace.$this->namespaceVersion.$key) > $this->maxIdLength) {
$id = $this->namespace.$this->namespaceVersion.substr_replace(base64_encode(hash('sha256', $key, true)), ':', -22);
$adjustedKey = substr_replace(base64_encode(hash('sha256', $key, true)), ':', -(strlen($this->namespaceVersion) + 22));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for the line split, we're ok with longer lines. Could you add a \ just before strlen also please?

@kshtompel
Copy link
Author

@nicolas-grekas I can add tests for AbstractAdapter but I can do it in a day. Is it ok?

@nicolas-grekas
Copy link
Member

Sure, thank you!

@nicolas-grekas
Copy link
Member

Thank you @kshtompel.

@nicolas-grekas nicolas-grekas merged commit ba8b63b into symfony:3.4 Jul 1, 2018
nicolas-grekas added a commit that referenced this pull request Jul 1, 2018
…versioning enabled (Constantine Shtompel)

This PR was squashed before being merged into the 3.4 branch (closes #27752).

Discussion
----------

[Cache] provider does not respect option maxIdLength with versioning enabled

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #27746
| License       | MIT
| Doc PR        |

Component symfony/cache generates cache item ID longer then maxIdLength when versioning is enabled

Commits
-------

ba8b63b [Cache] provider does not respect option maxIdLength with versioning enabled
@nicolas-grekas
Copy link
Member

And congrats for your first commit in Symfony :)

This was referenced Jul 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants