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

Undefined variable $value #6304

Open
da411d opened this issue Oct 15, 2024 · 4 comments
Open

Undefined variable $value #6304

da411d opened this issue Oct 15, 2024 · 4 comments

Comments

@da411d
Copy link

da411d commented Oct 15, 2024

if ($value !== FALSE && $this->_redis->sIsMember('_ci_redis_serialized', $key))

@jamieburchell
Copy link
Contributor

jamieburchell commented Oct 17, 2024

Maybe @exussum12 knows

I can't figure out what this is supposed to be doing as the code it replaces returned the unserialized value in that case.

@exussum12
Copy link
Contributor

exussum12 commented Oct 17, 2024

Looks like it was refactored

                $value = $this->_redis->get($key);

		if ($value !== FALSE && $this->_redis->sIsMember('_ci_redis_serialized', $key))
		{
			return unserialize($value);
		}

Was the original. It was there for backwards compatibility between versions. There used to be a key in redis which stored all other keys, that made redis slow for large caches.

The current code (without any testing) looks like it will have the same issue of scaling for large caches.

Happy to remove the value (as its never used) though would check it scales with 1m cache entries for example

@jamieburchell
Copy link
Contributor

jamieburchell commented Oct 17, 2024

I think how it was before this merge makes most sense, but I don't have any experience with this code and don't use Redis myself.

@exussum12
Copy link
Contributor

The PR which was merged

https://github.com/bcit-ci/CodeIgniter/pull/5752/files

The context around where that PR came from

#5739

The context of the PR, I think all of the code which interacts with _ci_redis_serialized can be removed - it looks like BC between versions and will have been long enough now to now have an issue with removing it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants
@exussum12 @jamieburchell @da411d and others