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

Use "json_encode" and "json_decode" instead of unserialize #18762

Merged
merged 1 commit into from
Sep 4, 2015

Conversation

LukasReschke
Copy link
Member

@blizzz Please review.

Fixes #16498

@LukasReschke LukasReschke added this to the 8.2-current milestone Sep 2, 2015
@scrutinizer-notifier
Copy link

A new inspection was created.

@RobinMcCorkell
Copy link
Member

👍👍👍👍👍👍👍👍

@blizzz
Copy link
Contributor

blizzz commented Sep 3, 2015

@LukasReschke migration?

@LukasReschke
Copy link
Member Author

@blizzz As far I see this uses the memcache, if you update to a new release the caching keys will have a new prefix (including the version) and thus there is no migration required.

@blizzz
Copy link
Contributor

blizzz commented Sep 3, 2015

@LukasReschke never mind. I mixed it up, overnight, with settings. I give it a practical try later, so far stuff looks good.

@MorrisJobke
Copy link
Contributor

Tested and still works with LDAP. 👍

MorrisJobke added a commit that referenced this pull request Sep 4, 2015
Use "json_encode" and "json_decode" instead of unserialize
@MorrisJobke MorrisJobke merged commit c9457fd into master Sep 4, 2015
@MorrisJobke MorrisJobke deleted the use-json-instead-of-unserialize branch September 4, 2015 09:40
@MorrisJobke
Copy link
Contributor

@LukasReschke @blizzz Would it make sense to backport this to stable8.1 to mitigate the unserialize issue in https://github.com/owncloud/enterprise/issues/1094? Or is this too risky?

@blizzz
Copy link
Contributor

blizzz commented Feb 24, 2016

@MorrisJobke if you include the other fix (the missing true), it's low risk.

@MorrisJobke
Copy link
Contributor

@MorrisJobke if you include the other fix (the missing true), it's low risk.

Sure. I just think about migration implications.

@LukasReschke After an upgrade the cache gets cleared completely, right?

@MorrisJobke
Copy link
Contributor

@LukasReschke After an upgrade the cache gets cleared completely, right?

Or better: it will use the version for cached entries and therefore not reuse the cache that was created before the update. Is this correct?

@MorrisJobke
Copy link
Contributor

Regression fix: #21999

@MorrisJobke
Copy link
Contributor

@MorrisJobke if you include the other fix (the missing true), it's low risk.

#22632

@lock
Copy link

lock bot commented Aug 7, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Get rid of unserialize in LDAP caching
5 participants